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.

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

Add support for the Zenoh Client's usage of the micro URI #75

Merged

Conversation

gregmedd
Copy link
Contributor

@gregmedd gregmedd commented Mar 28, 2024

Per spec, the Zenoh client uses the micro URI serialization for UUri. However, it has support for one extra ** UUri mode that requires the ability to serialize only the UAuthority.

This change splits out the UAuthority serialization into its own static method, which can be used both when serializing full UUris and when generating Zenoh keys for the ** mode.

Note: This PR is dependent on #73 and cannot be merged until that PR is complete.

@gregmedd
Copy link
Contributor Author

gregmedd commented Apr 3, 2024

This PR can be reviewed now. However, only the last commit (8d1fac3) should be reviewed. All commits before that are from #73. Once #73 is merged, I'll take this out of draft state so it's clear that it is ready to go.

Copy link
Contributor

@debruce debruce left a comment

Choose a reason for hiding this comment

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

I think this looks okay, and surely represents a large amount of work. However, I think it points to wider design issues, especially around the use of static methods instead of constructors, error propagation, absence of hashing methods for objects likely to be used as keys, etc.

@gregmedd
Copy link
Contributor Author

gregmedd commented Apr 4, 2024

... surely represents a large amount of work.

This PR can be reviewed now. However, only the last commit (8d1fac3) should be reviewed. All commits before that are from #73. Once #73 is merged, I'll take this out of draft state so it's clear that it is ready to go.

The Zenoh client needs to serialize _just_ the UAuthority under some
circumstances. To support this, that portion of
MicroUriSerializer::serialize() has been split out into its own separate
overload that is used both by the main UUri serializer and the Zenoh key
converter.
@gregmedd gregmedd force-pushed the feature/1.5.7/zenoh-micro-uri branch from 8d1fac3 to 0118557 Compare April 4, 2024 21:10
@gregmedd gregmedd marked this pull request as ready for review April 4, 2024 21:11
@stevenhartley stevenhartley merged commit bc5c7de into eclipse-uprotocol:main Apr 4, 2024
3 checks passed
@gregmedd gregmedd deleted the feature/1.5.7/zenoh-micro-uri branch April 5, 2024 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants