Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add UUID Validator and its unit tests #128

Merged
merged 1 commit into from
Jun 6, 2024
Merged

Add UUID Validator and its unit tests #128

merged 1 commit into from
Jun 6, 2024

Conversation

BishrutSubedi
Copy link
Contributor

@BishrutSubedi BishrutSubedi commented May 29, 2024 •
edited
Loading

This PR intends to add UUID validator, and it's unit tests in eclipse up-cpp

@gregmedd gregmedd linked an issue May 29, 2024 that may be closed by this pull request
Implement and test datamodel::validator::Uuid #118
Closed
@gregmedd gregmedd self-requested a review May 29, 2024 23:55
pranavishere2
pranavishere2 reviewed May 30, 2024
View reviewed changes
Copy link
Contributor

@pranavishere2 pranavishere2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we change the name "TestFixture" to some meaningful name. One of the reason is to have meaningful name is to provide flexibility to run specific set of testcases by applying filters and not the whole up-cpp project testcases.

gregmedd
gregmedd requested changes Jun 5, 2024
View reviewed changes
src/datamodel/validator/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/validator/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/validator/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/validator/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/validator/Uuid.cpp Outdated Show resolved Hide resolved
test/coverage/datamodel/UuidValidatorTest.cpp Outdated Show resolved Hide resolved
test/coverage/datamodel/UuidValidatorTest.cpp Outdated Show resolved Hide resolved
test/coverage/datamodel/UuidValidatorTest.cpp Outdated Show resolved Hide resolved
test/coverage/datamodel/UuidValidatorTest.cpp Outdated Show resolved Hide resolved
test/coverage/datamodel/UuidValidatorTest.cpp Show resolved Hide resolved
gregmedd
gregmedd reviewed Jun 6, 2024
View reviewed changes
Copy link
Contributor

@gregmedd gregmedd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more small comments. The minor things don't need to be done, but they'd be nice.

src/datamodel/validator/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/validator/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/validator/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/validator/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/validator/Uuid.cpp Outdated Show resolved Hide resolved
billpittman
billpittman suggested changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@billpittman billpittman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good, when dealing with bitmasks and shift values, I tend to try to single source that code as much as possible in the hopes of reducing mistakes. See inline comments.

src/datamodel/validator/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/validator/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/validator/Uuid.cpp Outdated Show resolved Hide resolved
@BishrutSubedi BishrutSubedi changed the title UUID Validator and it's unit test Add UUID Validator and its unit tests Jun 6, 2024
@BishrutSubedi
Add Uuid validator and its unit tests
936dae4 
This PR adds UUID validator for uprotocol uuid v8

Constexpr and internal method are implemented to extract right
fields of uuid by bitshifting.

Time arithmetic is done on chrono literals
billpittman
billpittman approved these changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@billpittman billpittman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes, looks great!

gregmedd
gregmedd approved these changes Jun 6, 2024
View reviewed changes
Copy link
Contributor

@gregmedd gregmedd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested changes complete, code looks good, commit message and history clean. Approved.

@gregmedd gregmedd merged commit 1102083 into eclipse-uprotocol:main Jun 6, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregmedd gregmedd gregmedd approved these changes

@billpittman billpittman billpittman approved these changes

@pranavishere2 pranavishere2 Awaiting requested review from pranavishere2

Assignees

@BishrutSubedi BishrutSubedi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement and test datamodel::validator::Uuid
4 participants
@BishrutSubedi @billpittman @gregmedd @pranavishere2