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

rename wrapped zenoh-protocol structures to ...Proto, wrap and expose EntityGlobalId #1118

Merged
merged 13 commits into from
Jun 12, 2024

Conversation

milyin
Copy link
Contributor

@milyin milyin commented Jun 11, 2024

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:

  • structures which are completely internal for zenoh crate should have suffix Inner (this is the case for some shm structs)
  • structures which are used by extra libraries / tools, like structures exposed by zenoh-protocol should have suffix corresponding to crate name, like ...Proto
  • corresponding wrapper API structures reexported by zenoh have no suffix and located now in zenoh-config::wrappers crate

This PR proposes updates above for ZenohId, EntityGlobalId and Hello

Also it moves ZenohId from config:: to info::. This seems more logical

@milyin milyin requested a review from Mallets June 11, 2024 14:37
@milyin milyin marked this pull request as ready for review June 11, 2024 15:01
@milyin milyin changed the title Entity id2 rename wrapped internal structures to ...Inner, wrap and expose EntityGlobalId Jun 11, 2024
@milyin milyin changed the title rename wrapped internal structures to ...Inner, wrap and expose EntityGlobalId rename wrapped zenoh-protocol structures to ...Proto, wrap and expose EntityGlobalId Jun 11, 2024
@@ -129,3 +130,39 @@ impl Hello {
}
}
}

/// A zenoh Hello message.
pub struct Hello(HelloProto);
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

@Mallets Mallets Jun 12, 2024

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?

Copy link
Contributor Author

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

@Mallets Mallets merged commit c8537bc into dev/1.0.0 Jun 12, 2024
17 of 21 checks passed
@Mallets Mallets deleted the entity_id2 branch June 12, 2024 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants