rust-electrum-client icon indicating copy to clipboard operation
rust-electrum-client copied to clipboard

Rename `use-rustls` and `use-openssl` features

Open dr-orlovsky opened this issue 4 years ago • 4 comments

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

dr-orlovsky avatar Jan 21 '21 14:01 dr-orlovsky

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 avatar Jan 21 '21 14:01 afilini

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.

afilini avatar Jan 21 '21 14:01 afilini

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

dr-orlovsky avatar Jan 21 '21 15:01 dr-orlovsky

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.

afilini avatar Jan 21 '21 15:01 afilini