-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Simplify checking feature syntax #14106
Conversation
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.
Looks good, just some bikeshedding on the name.
src/cargo/core/summary.rs
Outdated
} | ||
} | ||
|
||
fn dep_name(&self) -> InternedString { |
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.
I think this might be a confusing name, since FeatureValue::Feature
is not necessarily a dependency name. For example:
[features]
foo = ["bar"]
bar = []
Here the FeatureValue::Feature("bar")
is not a dependency name.
I realize this is refactoring code that was already using dep_name
, but the original was somewhat abusing the nomenclature.
I would go with feature_or_dep_name
to make it clearer it can be either. What do you think?
src/cargo/core/summary.rs
Outdated
/// Returns `true` if this feature explicitly used `dep:` syntax. | ||
pub fn has_dep_prefix(&self) -> bool { | ||
matches!(self, FeatureValue::Dep { .. }) | ||
fn explicitly_name(&self) -> Option<InternedString> { |
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.
Here name
seems ambiguous to me. It could be a "feature name" or a "dependency name". I might go with something like explicit_dep_name
or something like that.
Also, can you add a docstring to say exactly what it does?
f0ab501
to
ea3c207
Compare
ea3c207
to
939a114
Compare
|
I've not checked the function logic to see if it would work (mostly out for the next couple weeks) so I was just noting potential applications, one being in Cargo. |
Thanks! @bors r+ |
☀️ Test successful - checks-actions |
1 similar comment
☀️ Test successful - checks-actions |
👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request. |
Update cargo 17 commits in 3ed207e416fb2f678a40cc79c02dcf4f936a21ce..bc89bffa5987d4af8f71011c7557119b39e44a65 2024-06-18 19:18:22 +0000 to 2024-06-22 00:36:36 +0000 - test: migrate weak_dep_features, workspaces and yank to snapbox (rust-lang/cargo#14111) - test: migrate features and features(2|_namespaced) to snapbox (rust-lang/cargo#14100) - test: Add auto-redaction for not found error (rust-lang/cargo#14124) - test: migrate build to snapbox (rust-lang/cargo#14068) - test: migrate unit_graph, update and vendor to snapbox (rust-lang/cargo#14119) - fix(test): Un-redact Packaged files (rust-lang/cargo#14123) - test: Auto-redact file number (rust-lang/cargo#14121) - test: migrate lints_table and lints/(mod|unknown_lints) to snapbox (rust-lang/cargo#14104) - Simplify checking feature syntax (rust-lang/cargo#14106) - test: migrate testsuites to snapbox (rust-lang/cargo#14091) - Make `-Cmetadata` consistent across platforms (rust-lang/cargo#14107) - fix(toml): Warn when edition is unuset, even when MSRV is unset (rust-lang/cargo#14110) - Add `CodeFix::apply_solution` and impl `Clone` (rust-lang/cargo#14092) - test: migrate `cargo_alias_config&cargo_config/mod` to snapbox (rust-lang/cargo#14093) - Simplify checking for dependency cycles (rust-lang/cargo#14089) - test: Migrate `pub_priv.rs` to snapshot (rust-lang/cargo#14103) - test: migrate rustdoc and rustdocflags to snapbox (rust-lang/cargo#14098) <!-- r? ghost -->
Update cargo 17 commits in 3ed207e416fb2f678a40cc79c02dcf4f936a21ce..bc89bffa5987d4af8f71011c7557119b39e44a65 2024-06-18 19:18:22 +0000 to 2024-06-22 00:36:36 +0000 - test: migrate weak_dep_features, workspaces and yank to snapbox (rust-lang/cargo#14111) - test: migrate features and features(2|_namespaced) to snapbox (rust-lang/cargo#14100) - test: Add auto-redaction for not found error (rust-lang/cargo#14124) - test: migrate build to snapbox (rust-lang/cargo#14068) - test: migrate unit_graph, update and vendor to snapbox (rust-lang/cargo#14119) - fix(test): Un-redact Packaged files (rust-lang/cargo#14123) - test: Auto-redact file number (rust-lang/cargo#14121) - test: migrate lints_table and lints/(mod|unknown_lints) to snapbox (rust-lang/cargo#14104) - Simplify checking feature syntax (rust-lang/cargo#14106) - test: migrate testsuites to snapbox (rust-lang/cargo#14091) - Make `-Cmetadata` consistent across platforms (rust-lang/cargo#14107) - fix(toml): Warn when edition is unuset, even when MSRV is unset (rust-lang/cargo#14110) - Add `CodeFix::apply_solution` and impl `Clone` (rust-lang/cargo#14092) - test: migrate `cargo_alias_config&cargo_config/mod` to snapbox (rust-lang/cargo#14093) - Simplify checking for dependency cycles (rust-lang/cargo#14089) - test: Migrate `pub_priv.rs` to snapshot (rust-lang/cargo#14103) - test: migrate rustdoc and rustdocflags to snapbox (rust-lang/cargo#14098) <!-- r? ghost -->
What does this PR try to resolve?
Similar to #14089, in my PubGrub work I had to try to understand the
build_feature_map
code that determines if the name refers to a feature or an optional dependency. There were a couple of little things I wanted to clean up while I was staring at the code. Specifically:bail
that could be in line at the format string.split_once
could replace afind ... split_at
dance.dep_map
we were constructing a fullVec<Dependency>
, when we only cared about whether there was any dependency and whether any of the dependencies were optional.How should we test and review this PR?
It's an internal re-factor, and the tests still pass.
Additional information
If you're having questions about this code while you're reviewing, this would be a perfect opportunity for better naming and comments. Please ask questions.
@epage: After this PR I am likely to copy this code into my pubgrub tester. Are there other users who might be interested in looking at a cargo.toml or an index entry and determining what feature names are available and what dependencies they enable? If so maybe this function should be moved to one of the new stable-ish reusable libraries.