-
Notifications
You must be signed in to change notification settings - Fork 173
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
rename wrapped zenoh-protocol structures to ...Proto
, wrap and expose EntityGlobalId
#1118
Conversation
...Inner
, wrap and expose EntityGlobalId
...Inner
, wrap and expose EntityGlobalId
...Proto
, wrap and expose EntityGlobalId
@@ -129,3 +130,39 @@ impl Hello { | |||
} | |||
} | |||
} | |||
|
|||
/// A zenoh Hello message. | |||
pub struct Hello(HelloProto); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect this type to be defined in the API and not in zenoh-protocol
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the one hand this is right, but sometimes it doesn't work. Especially this doesn't work for ZenohId
: it's a wrapper for ZenohIdProto
and logically should be in zenoh
, but it's also used by zenoh-config
so it should be outside of zenoh
to avoid circular dependency.
So the approach when all wrappers for reexported structures should be located near the original structure is systematic and guarantees no dependency problems. Another solution would be to add special crate for wrapped structures which can be reused by other crates. But 1) in some rare cases this also can cause circular dependency and 2) this would require adding one more crate, increasing complexity of our project
So I think that the approach when reexported wrappers are declared near the original structures is the most acceptable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the real problem is that zenoh-config
uses API-level types directly from zenoh-protocol
. But zenoh-protocol
per-se does not need to care about those wrapper.
In zenoh-protocol
everything is defined and exported as public to detect as many breaking changes as possible throughout the codebase as soon as one minimal change happens. Adding the wrapper inside zenoh-protocol
may lead to more misusage to me than anything else. Could the wrapper be defined in zenoh-config
and then re-exported by zenoh
and then leave untouched the zenoh-protocol
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that's another solution. Let's put all such wrappers to zenoh-config
. This will cause reexport duplication at this moment because we reexporting zenoh-config::*
in config
crate and also should reexport Hello
in scoiting
, etc. But I think this is not a big issue at this moment, but better to make reexport from zenoh-config
explicit in 1.0.0
There are structures with public fields and/or public methods which should not be in the API in the internal crates. These structures should be also exported in the API without these public items. This update proposes common rule how tho handle such structures:
Inner
(this is the case for some shm structs)...Proto
zenoh-config::wrappers
crateThis PR proposes updates above for
ZenohId
,EntityGlobalId
andHello
Also it moves
ZenohId
fromconfig::
toinfo::
. This seems more logical