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 use-rustls and use-openssl features #42

Open
dr-orlovsky opened this issue Jan 21, 2021 · 4 comments
Open

Rename use-rustls and use-openssl features #42

dr-orlovsky opened this issue Jan 21, 2021 · 4 comments

Comments

@dr-orlovsky
Copy link
Contributor

According to Rust API guidelines feature names with use- prefix considered wrong: https://rust-lang.github.io/api-guidelines/naming.html#c-feature

This is justified, since ppl looking for features will frequently use rustls instead of use-rustls and will report issues - we had plenty of them in rust-bitcoin with similar serde situation.

There is two options:

  1. Rename crates with rustls_crate = { package = "rustls" } + extern crate rustls_crate as rustls in lib.rs and rename features into rustls
  2. Rename features into tls and ssl

I am pro second option

@afilini
Copy link
Member

afilini commented Jan 21, 2021

I think option (2) would create even more confusion, because in many ways tls and ssl are basically considered synonyms. Even worse, I was also planning to add support for native-tls, and with that kind of naming scheme I wouldn't know how to introduce it.

Option (1) makes sense, but I'd rather not break the naming for downstream users of the library. We can keep this issue open an potentially do this as part of a larger refactoring that would already break the naming of features for instance.

Other than that, I consider this very low priority

@afilini
Copy link
Member

afilini commented Jan 21, 2021

By the way, if I read the link you posted correctly it's not even necessary to rename crates: it seems that cargo is smart enough to map features with the same name of a crate to enabling that crate. In their example they show that by making serde optional and then when enabling the serde feature cargo understands that it should include the optional dependency.

@dr-orlovsky
Copy link
Contributor Author

No, that would not work :( You will also need to enable serde in bitcoin with bitcoin/use-serde

@afilini
Copy link
Member

afilini commented Jan 21, 2021

Well in our case we don't really need to have serde optional, but I checked now and it looks like for rustls we also have to include other dependencies, so yeah that can't work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants