zenoh icon indicating copy to clipboard operation
zenoh copied to clipboard

refactor: keep only trait in prelude and move main types in root

Open wyfo opened this issue 1 year ago • 3 comments

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.

wyfo avatar May 02 '24 15:05 wyfo

Following a discussion with @milyin. Don't hesitate to ping other people interested in it.

@Mallets

wyfo avatar May 02 '24 15:05 wyfo

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(...);

milyin avatar May 03 '24 13:05 milyin

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 avatar May 07 '24 10:05 wyfo

@wyfo Please change the base branch to dev/1.0.0 as protocol_changes is no longer the development branch.

fuzzypixelz avatar May 17 '24 10:05 fuzzypixelz

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

wyfo avatar May 28 '24 12:05 wyfo

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.

Mallets avatar May 28 '24 12:05 Mallets

I've rebased the PR and updated test imports

wyfo avatar May 29 '24 09:05 wyfo

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.

wyfo avatar May 29 '24 13:05 wyfo

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.

@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.

fuzzypixelz avatar May 29 '24 13:05 fuzzypixelz

I don't know why the CI fails this time. @fuzzypixelz do you have an idea?

wyfo avatar May 30 '24 08:05 wyfo