-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix: improve message for inactive weak optional feature with edition2024 #14019
Conversation
r? @weihanglo rustbot has assigned @weihanglo. Use |
@@ -221,7 +221,7 @@ Caused by: | |||
error: failed to parse manifest at `[ROOT]/foo/Cargo.toml` | |||
|
|||
Caused by: | |||
feature `feat` includes `dep?/feat`, but `dep` is not a dependency | |||
feature `feat` includes `dep?/feat`, activate it in a feature with `dep:dep` if `dep` is an enabled dependency |
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.
if
dep
is an enabled dependency
I feel the wording could be improved here. To help in coming up with ideas, could you expand on what you are trying to convey
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.
Yes, the messge need to include the cases that dependency inactive and dependency missing, if dep is an enabled dependency
means the former.
#14026 is another PR to distinguish these. And this PR may require more detailed wording to address both cases.
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 suggest keep this PR to polish the message unless #14026 is merged
Caused by: | ||
feature `feat` includes `dep?/feat`, but `dep` is not a dependency | ||
feature `feat` includes `dep?/feat`, activate it in a feature with `dep:dep` if `dep` is an enabled dependency |
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.
This feels like a regression for us to be telling people to add dep:dep
when they also need to add the dependency itself.
☔ The latest upstream changes (presumably #14028) made this pull request unmergeable. Please resolve the merge conflicts. |
Close this in favor of #14026 |
What does this PR try to resolve?
This will improve the message for inactive weak optional feature with edition2024.
This doesn't distinguish whether the dependency had set, which needs to get the dependency from origin that will make more complex.
Fixes #14015
How should we test and review this PR?
one commit add the test, one commit fix and update the test
Additional information