-
Notifications
You must be signed in to change notification settings - Fork 40
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
Make _danger-local-https feature additive #430
Conversation
6b17604
to
05d0406
Compare
Pull Request Test Coverage Report for Build 12537462995Details
💛 - Coveralls |
5e87fe5
to
580a92a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the code change looks reasonable, but am missing the correspondence between these changes and the following:
Before this change
io::fetch_ohttp_keys
changed its signature based
on the feature flag. I noticed downstream that this creates a feature
flag mess in bindings and it's one opportunity to address tech debt.
does this just boil down to splitting things so instead of a cfg gated argument to a fn it's a cfg gated fn? if so, then ACK, but if not then i misunderstood something. either way explicitly describing the problem that existed before this commit would be helpful, as "a flag mess in the bindings" is a bit vague
It does. A flag mess in the bindings meant that we needed to have switched function signatures downstream as well. Do you need me to change the commit message? |
580a92a
to
fdd1117
Compare
oops missed this, thanks for re-requesting review. i see... i don't think it matters much to change the commit message, it helps me to understand in this case, and in general that's a good thing for commit messages to have, but i also doubt this particular explanation will be very meaningful to anyone reviewing the history in hindsight. i'm ACK, but feel free to do that if for extra cookie points ;-) |
Instead of feature gating function params, which introduces the need for similar feature gating in downstream code, feature gate individual functions. See: <https://doc.rust-lang.org/cargo/reference/features.html#feature-unification> "Rust features should be *additive*. That is, enabling a feature should not disable functionality, and it should usually be safe to enable any combination of features" Before this change `io::fetch_ohttp_keys` had two function signatures and changed its signature based on the feature flag. I noticed downstream that this creates a necessity to carry those flags mess into bindings or else face compile issues under certain feature combinations. `io` now enables the `v2` feature since it only contains methods for fetching OHTTP keys which is only useful in V2. In the future perhaps it will also include generic HTTP client behavior taking `Request` as a parameter in which case it should not enable `v2`, because such functionality would be useful in a V1 setting as well.
fdd1117
to
fdcc566
Compare
#430 made some features additive, but didn't properly address the tests to run under the minimal requirements nor the scripts to run the tests. This fixes that
See: https://doc.rust-lang.org/cargo/reference/features.html#feature-unification
"Rust features should be additive. That is, enabling a feature should not disable functionality, and it should usually be safe to enable any combination of features"
Before this change
io::fetch_ohttp_keys
changed its signature based on the feature flag. I noticed downstream that this creates a feature flag mess in bindings and it's one opportunity to address tech debt.This begins to address #392