-
Notifications
You must be signed in to change notification settings - Fork 141
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
refactor: keep only trait in prelude #1007
Conversation
I don't think that we have to be such conservative for prelude. Zenoh is big, but not as big, as e.g. rust standard library. From my point of view the goal of prelude should be to allow user to import only One of possible solutions could be like this: pub mod prelude {
pub mod traits {
// reexport all the traits
}
pub mod types {
// list here the big API element which we consider as core API
}
mod ztypes { // !!! not public !!!
// exactly same as types, but commonly used names prefixed with Z: Err is ZErr, result is ZResult
}
pub use traits::*;
pub use ztypes::*;
}
pub use prelude::types::*; With this scheme we don't need to think too much what to include to prelude and what to include to root and what do not. Variants of usage: use zenoh::prelude::*;
Config config;
let session = zopen(config, ...);
let subscriber = session.declare_subscliber(...); use zenoh::prelude::traits::*;
zenoh::Config config;
let session = zenoh::open(config, ...);
let subscriber = session.declare_subscliber(...); use zenoh::SessionDeclarations; // for trait `declare_subscriber`
zenoh::Config config;
let session = zenoh::open(config, ...);
let subscriber = session.declare_subscliber(...); |
@wyfo Please change the base branch to dev/1.0.0 as protocol_changes is no longer the development branch. |
// Expose some functions directly to root `zenoh::`` namespace for convenience | ||
pub use crate::api::{scouting::scout, session::open}; | ||
#[doc(inline)] | ||
pub use crate::{ |
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'm not against but not 100% convinced wether it's a good idea to export all those types directly under zenoh::*
. @milyin any opinion?
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.
Being able to write zenoh::Result
/zenoh::Error
is kind of mandatory IMO.
Also a lot of common english names, you may not want to import them directly, but use the module instead, for example zenoh::Session
, or zenoh::Priority
. I think it's more convenient for the user (in my own code, I think I would use zenoh::Session
more than Session
).
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 agree that some user may prefer zenoh::Session
to just Session
. My point is more on what do we decide to put in the root? And what not? E.g., Session
, Result
, Error
make sense to be in the root but I'm not sure about Priority
(which is just taken as an example here). Is Priority
really a first class type to be exported in the root? Are we going to put all types in the root loosing any kind of hierarchy/grouping?
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 can split the PR in two, one for the prelude, and one draft for the putting the types into the root. The only issue is about imports in the examples.
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 agree that this reexport to root should be shorter. Otherwise we'll be drowned in root types. I'd keep only commonly used functions, like open
, scout
, commonly used types types like Result
and Error
and types with the name same as it's module, e.g. KeyExpr
, Session
, etc. to avoid repeating like zenoh::session::Session
. But for example it's important to not reexportQuery
to force user to write use zenoh::queryable::Query
and let him immediately see that this is unrelated to same-named query
module.
I agree that better to split it to 2 PRs: one for prelude and one for global reexport
About imports from zenoh dependencies in tests, I didn't write them but used my IDE auto-import. I don't think it matters a lot in tests, as it's not public code, but I can modify them if wanted |
I agree that it doesn't matter a lot in the tests but it would be good to verify they can actually be imported from |
I've rebased the PR and updated test imports |
The error in the CI is a false positive, |
@wyfo You're experiencing the wonderful phenomenon of "Shadow Merges" in GitHub pull requests. The CI does not run on your branch as such, but on different version of it where a I can confirm that someone on |
I don't know why the CI fails this time. @fuzzypixelz do you have an idea? |
The current prelude exposes all the types of the API. This is considered as a bad practice, as many of these names are quite common and could conflict. Most of the popular crate using prelude are only putting traits in it (plus some types with particular names).
These PR removes all types from the prelude, and put the main ones directly in the root.