-
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
Use listed dependency name for feature names #5811
Use listed dependency name for feature names #5811
Conversation
r? @matklad (rust_highfive has picked a reviewer for you, use r? to override) |
r? @ehuss |
src/cargo/core/dependency.rs
Outdated
@@ -209,7 +209,11 @@ impl Dependency { | |||
&self.inner.req | |||
} | |||
|
|||
pub fn name(&self) -> InternedString { | |||
pub fn name_in_toml(&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.
Would you mind adding a short docstring to name_in_toml
, package_name
, and rename
? Perhaps with a little hint on when to use which? I suspect future me is going to forget and be confused.
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.
Sure thing!
9f3910c
to
a8f3bf8
Compare
@bors: r=ehuss |
📌 Commit a8f3bf8aab2d1c90442d4c0ac5894e3a27503bc4 has been approved by |
src/cargo/core/dependency.rs
Outdated
/// Usually this is what's written on the left hand side of a dependencies | ||
/// section, but it can also be renamed via the `package` key. | ||
/// | ||
/// Both of the dependencies below return `foo` for `name_in_toml`: |
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.
"for package_name
", surely?
This commit updates the implementation of renamed dependencies to use the listed name of a dependency in Cargo.toml for the name of the associated feature, rather than using the package name. This'll allow disambiguating between different packages of the same name and was the intention all along! Closes rust-lang#5753
a8f3bf8
to
5295cad
Compare
@bors: r=ehuss |
📌 Commit 5295cad has been approved by |
/// The renamed name of this dependency, if any. | ||
/// | ||
/// If the `package` key is used in `Cargo.toml` then this returns the same | ||
/// value as `name_in_toml`. |
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 don't want to stall this PR, but after several attempts I still can't wrap my head around this explanation.
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.
Ah sorry about that! I'll try to get around to cleaning this up soon.
⌛ Testing commit 5295cad with merge ac037ab4e8d7bef2735d0d3795a5e9bb2cb4860a... |
💥 Test timed out |
@bors: retry |
Use listed dependency name for feature names This commit updates the implementation of renamed dependencies to use the listed name of a dependency in Cargo.toml for the name of the associated feature, rather than using the package name. This'll allow disambiguating between different packages of the same name and was the intention all along! Closes #5753
☀️ Test successful - status-appveyor, status-travis |
This commit updates the implementation of renamed dependencies to use the listed
name of a dependency in Cargo.toml for the name of the associated feature,
rather than using the package name. This'll allow disambiguating between
different packages of the same name and was the intention all along!
Closes #5753