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

add 'open' function without parameters for create session with default config #805

Closed
milyin opened this issue Mar 10, 2024 · 7 comments
Closed
Labels
api sync Synchronize API with other bindings enhancement Existing things could work better release Part of the next release

Comments

@milyin
Copy link
Contributor

milyin commented Mar 10, 2024

Describe the feature

@kydos wrote "I think we should have an overloaded zenoh::open() that does not take the config. Having to create a config just to pass it seems an avoidable step."

@milyin milyin added the new feature Something new is needed label Mar 10, 2024
@milyin
Copy link
Contributor Author

milyin commented Mar 10, 2024

This is possible in C/C++, but for Rust the name should be different. This could be Session::default() for example.
BTW: why it was decided to have just standalone open function, not the Session::open or Session::new ?

@milyin milyin moved this to Backlog in Zenoh 1.0.0 release Mar 12, 2024
@milyin milyin added enhancement Existing things could work better api sync Synchronize API with other bindings and removed new feature Something new is needed labels Mar 12, 2024
@milyin milyin added release Part of the next release api fix Correct API and removed api fix Correct API labels May 29, 2024
@milyin
Copy link
Contributor Author

milyin commented May 30, 2024

This could be implemented this way:

let s = zenoh::open().wait()?;
...
let c = Config::from_file(file)?;
let s = zenoh::open().config(c).wait()?;

I.e. make open function without parameters and add .config() setter to OpenBuilder

@Mallets what do you think?

@Mallets
Copy link
Member

Mallets commented Jun 10, 2024

The proposal looks good to me and makes the API more coherent.

@wyfo
Copy link
Contributor

wyfo commented Jun 11, 2024

On my side, I'm against, because I don't think we want the user to use default config. I don't know our user use cases at all, but is there some users that are really using default config in production?

About coherence, I don't think having configuration as a required parameter is an inconsistency. Other methods also have required parameter, like keyexpr for declare_subscriber. The question is again, do we want to encourage user to use the default configuration?

@wyfo
Copy link
Contributor

wyfo commented Jun 17, 2024

Maybe scout could be a better candidate for this kind signature change?

@Mallets
Copy link
Member

Mallets commented Jun 25, 2024

According to latest discussion, zenoh::open() will keep accepting Config as of Today.
@milyin I believe we can close the issue.

@wyfo
Copy link
Contributor

wyfo commented Jun 25, 2024

@Mallets Is scout a better candidate for a signature rework?

@milyin milyin closed this as completed Aug 4, 2024
@github-project-automation github-project-automation bot moved this from Backlog to Done in Zenoh 1.0.0 release Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api sync Synchronize API with other bindings enhancement Existing things could work better release Part of the next release
Projects
Status: Done
Development

No branches or pull requests

3 participants