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

UUID serializer and its unit test #139

Merged
merged 1 commit into from
Jun 7, 2024
Merged

UUID serializer and its unit test #139

merged 1 commit into from
Jun 7, 2024

Conversation

BishrutSubedi
Copy link
Contributor

@BishrutSubedi BishrutSubedi commented Jun 1, 2024 •
edited
Loading

This PR adds UUID serializer for uprotocol uuid v8 #122

@BishrutSubedi BishrutSubedi changed the title Biss/UUID serializer UUID serializer and its unit test Jun 1, 2024
@gregmedd gregmedd linked an issue Jun 3, 2024 that may be closed by this pull request
Implement and test datamodel::serializer::Uuid #122
Closed
gregmedd
gregmedd requested changes Jun 3, 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.

For the most part, this looks good. I left a comment about the string format with a link to the relevant RFC, and suggested some additional test cases. Let me know if you have any questions.

include/up-cpp/datamodel/serializer/Uuid.h Outdated Show resolved Hide resolved
src/datamodel/serializer/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/serializer/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/serializer/Uuid.cpp Outdated Show resolved Hide resolved
test/coverage/datamodel/UuidSerializerTest.cpp Outdated Show resolved Hide resolved
test/coverage/datamodel/UuidSerializerTest.cpp Outdated Show resolved Hide resolved
test/coverage/datamodel/UuidSerializerTest.cpp Outdated Show resolved Hide resolved
test/coverage/datamodel/UuidSerializerTest.cpp Outdated Show resolved Hide resolved
gregmedd
gregmedd approved these changes Jun 5, 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.

I'm ok with this. There is one small note on a test case.

I don't think we should hold up the PR while we decide if we need the byte serializer.

src/datamodel/serializer/Uuid.cpp Outdated Show resolved Hide resolved
test/coverage/datamodel/UuidSerializerTest.cpp Show resolved Hide resolved
gregmedd
gregmedd requested changes Jun 5, 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.

Ok, I actually have just one more change to ask for. Can you rebase this onto main (make sure it is the latest main from eclipse-uprotocol/up-cpp and not your fork) so that the CI can run?

While you are doing that, I would also appreciate it if you could use interactive rebase to squash your commits and clean up the commit messages. You don't have to squash down to one commit - whatever number clearly shows the units of work.

This article provides some good guidance on commit messages.

@BishrutSubedi
Copy link
Contributor Author

BishrutSubedi commented Jun 6, 2024 •
edited
Loading

sounds good, I will squash to couple of commits

stevenhartley
stevenhartley approved these changes Jun 6, 2024
View reviewed changes
Copy link

@stevenhartley stevenhartley left a comment

Choose a reason for hiding this comment

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

LGTM

billpittman
billpittman suggested changes Jun 7, 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.

Mostly nitpicks - use your best judgement on those.

On the tests, looks great, I liked your use of sequential (expected, easy-to-identify) strings.

src/datamodel/serializer/Uuid.cpp Show resolved Hide resolved
src/datamodel/serializer/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/serializer/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/serializer/Uuid.cpp Outdated Show resolved Hide resolved
src/datamodel/serializer/Uuid.cpp Outdated Show resolved Hide resolved
test/coverage/datamodel/UuidSerializerTest.cpp Show resolved Hide resolved
test/coverage/datamodel/UuidSerializerTest.cpp Show resolved Hide resolved
test/coverage/datamodel/UuidSerializerTest.cpp Show resolved Hide resolved
@BishrutSubedi
Copy link
Contributor Author

Thank you, I've updated PR with the suggested changes

@billpittman
Copy link
Contributor

Thank you, I've updated PR with the suggested changes

No thank you. Looks great - thanks for the extra effort. Approved!

gregmedd
gregmedd approved these changes Jun 7, 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.

Looks good to me. Can you make sure that the commits are rebased so that the conan change is dropped? It should already be committed to main. Otherwise this is ready to go.

@BishrutSubedi
Add Uuid Serializer and its unit test
e80a719 
This PR adds uuid serializer for uprotocol uuid v8

AsString::serialize -> serializes uuid as string in human
redable format

AsString::deserialize -> deserializes string format uuid

AsBytes::serialize -> serializes uuid as bytes

AsBytes::deserialize -> deserialize uuid byte

The serialization and deserialization as byte checks the
endianess and makes sure uuid field are populated correctly
@gregmedd gregmedd merged commit 7b494f3 into eclipse-uprotocol:main Jun 7, 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

@stevenhartley stevenhartley stevenhartley approved these changes

@billpittman billpittman billpittman approved these changes

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::serializer::Uuid
4 participants
@BishrutSubedi @billpittman @stevenhartley @gregmedd