Skip to content

feat: always show list of features on cargo add #15658

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

Closed
wants to merge 1 commit into from

Conversation

motorailgun
Copy link
Contributor

Partially solves #15524

What does this PR try to resolve?

Currently, cargo shows list of features available for the package added only when features specified correctly. In turn, if the features specified by cargo add --features doesn't exist (at least for the version specified in Cargo.toml), it just bails with "unrecognized feature(s)" error.

This PR modifies the timing of print_dep_table_msg() so that it's always called.

How to test and review this PR?

The changes of UI can be observed by peeking the modified SVGs. I think those changes make sense, however there might be different opinions.

@rustbot
Copy link
Collaborator

rustbot commented Jun 13, 2025

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added Command-add S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 13, 2025
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the fix!

Could you squash these two commits into one commit? By doing so we can ensure every commit passes CI, and git bisect would be a bit easier to conduct.

@motorailgun
Copy link
Contributor Author

Thank you for reviewing! Just squashed and pushed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#15438 added the "similar feature" messages and the lists of disabled and enabled features that are showcased in these error messages. We even customized the displaying of those specifically for errors (collapsing enabled features while the regular display will collapse disabled features).

So this feels a bit redundant to show the feature list. Is there a reason you feel we should do both?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI our contrib docs recommend opening issues first so we can discuss these things before implementing so we know what direction to implement and avoid you going in a direction we might not end up going with

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops I somehow didn't notice that. I thought the "as of v x.x.x" part gives better insights in cases like the issue I mentioned, however it's undeniable that this feels too redundant.

And I'm sorry about discussion part - I hurried too much to do something; I should have talked on the issue beforehand.

Closing this PR, thanks for taking your time to review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Command-add S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants