zenoh
zenoh copied to clipboard
refactor: keep only trait in prelude and move main types in root
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.
Following a discussion with @milyin. Don't hesitate to ping other people interested in it.
@Mallets
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 prelude::* and still have access to key zenoh functionality.
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(...);
See https://www.reddit.com/r/rust/comments/1cle18j/to_prelude_or_to_not_to_prelude/ as well as https://users.rust-lang.org/t/to-use-prelude-or-to-not-to-use-prelude-that-is-the-question/110855
@wyfo Please change the base branch to dev/1.0.0 as protocol_changes is no longer the development branch.
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
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 zenoh without requiring importing the types from other crates.
I've rebased the PR and updated test imports
The error in the CI is a false positive, Value is used in zenoh/tests/session.rs in line 172
I don't know what to do. When I execute the command on my computer, there is no error.
The error in the CI is a false positive,
Valueis used inzenoh/tests/session.rsin line 172 I don't know what to do. When I execute the command on my computer, there is no error.
@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 git merge origin/dev/1.0.0 has been executed, yielding a different HEAD. This is done in the actions/checkout action.
I can confirm that someone on dev/1.0.0 removed the usage of Value in line 172. You should rebase/merge.
I don't know why the CI fails this time. @fuzzypixelz do you have an idea?