ferristseng / rust-ipfs-api

IPFS HTTP client in Rust
Apache License 2.0
247 stars 68 forks source link

allow hyper-rustls or no tls as alternatives for hyper-tls #64

Closed jcaesar closed 3 years ago

jcaesar commented 3 years ago

(Another attempt at @akru's #39.)

ferristseng commented 3 years ago

What's the reasoning behind removing the default? I think it might confuse people who are upgrading

jcaesar commented 3 years ago

No particular reason, I probably misread your comment on the previous PR. I reverted that part.

Though there is one thing that does bother me a bit: There is no easy way to say "I want rust-ipfs-api with hyper, but without any TLS implementation". It's also not entirely clear which of the optional dependencies are supposed to be used as features, and how they depend on each other.

If you want me to, I can make another PR, reordering that a bit, maybe like this:

[features]
default           = ["with-hyper-tls", "with-builder"]
with-hyper-tls    = ["with-hyper", "hyper-tls"]
with-hyper-rustls = ["with-hyper", "hyper-rustls"]
with-hyper        = ["hyper", "hyper-multipart-rfc7578", "failure"]
with-actix        = ["actix-http", "actix-multipart-rfc7578", "awc", "derive_more"]
with-builder      = ["typed-builder"]

That should make it clear how the features are supposed to be used.

([Edit:] There is a workaround for keeping the with- out of the feature names, but it does make things less obvious.)

ferristseng commented 3 years ago

If you want me to, I can make another PR, reordering that a bit, maybe like this:

Yeah, I think this works. I'm fine with the with- prefix since it's consistent.

Thank you!

jcaesar commented 3 years ago

Here we go. Since it kind of depends on this PR, I just appended a commit here.

ferristseng commented 3 years ago

Nice, this looks good. I'll update the docs before publishing a new build

jcaesar commented 3 years ago

Hm, just noticed that this maybe isn't too optional: features should be strictly additive, so adding more features to a compiling crate should not make it not compile. I think the solution would be to default to one if both are active. The sad part is that there doesn't seem to be a nice way to emit a warning in this case, and it will also make the cfg situation even more complicated, which you lament in #70. (Though I think there are ways to mitigate that.)

ferristseng commented 3 years ago

Yeah, I'm working on just having separate crates for each backend implementation which should simplify things a bit, but it's still pretty far off. It also won't fully solve the "mutually exclusive feature flag" issue, but it should make it way easier to understand

jcaesar commented 3 years ago

It wouldn't? If you have a trait IpfsClient and then an impl IpfsClient for IpfsClientHyper, either in a subcrate or behind a feature flag...?

ferristseng commented 3 years ago

Yeah because the TLS crates would still be mutually exclusive I think

jcaesar commented 3 years ago

IpfsClientHyperRusTLS? Mmmh. I suppose HyperConnector could also be a (struct) type argument (questionable whether to have three separate news, or one with a <T: Connect> parameter...)