Skip to content
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

Made hidden features hidden on cargo add when not activated #10878

Closed
wants to merge 4 commits into from

Conversation

johnmatthiggins
Copy link
Contributor

Fixes issue #10794.

@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 19, 2022
@johnmatthiggins johnmatthiggins changed the title Removed redundant logic Made hidden features hidden on cargo add when not activated Jul 19, 2022
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 your contribution!

Could you add some tests like the following?

https://github.com/rust-lang/cargo/tree/master/tests/testsuite/cargo_add

Here is the guide to adding UI tests: https://doc.crates.io/contrib/tests/writing.html#ui-tests

shell.write_stderr(format_args!(" {}\n", feat), &ColorSpec::new())?;
let is_hidden = feat.chars().nth(0).map(|c| c == '_').unwrap_or(false);

if !is_hidden {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this wasn't in the Issue but how about we also show these features when the user has increased the verbosity? We could use config.extra_verbose for now. Might be good to lower that to just verbose at a later point but that'll require some more rework because it isn't directly exposed

Copy link
Contributor Author

@johnmatthiggins johnmatthiggins Jul 21, 2022

Choose a reason for hiding this comment

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

I think this is a cool idea and I would love to implement it. However, given the existence of issue #10882 and my inexperience with this project, I'm not sure how to proceed with this PR. Should I wait for the requirements to be clearer before making any more changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend holding off until a determination has been made about #10794.

@epage
Copy link
Contributor

epage commented Jul 19, 2022

Could you add some tests like the following?

To add, we should make sure we cover all cases, like when the hidden features are activated and deactivated.

Copy link
Contributor

@epage epage left a comment

Choose a reason for hiding this comment

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

shell.write_stderr(&prefix, &ColorSpec::new())?;
shell.write_stderr('-', &ColorSpec::new().set_bold(true).set_fg(Some(Red)))?;
shell.write_stderr(format_args!(" {}\n", feat), &ColorSpec::new())?;
let is_hidden = feat.chars().nth(0).map(|c| c == '_').unwrap_or(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is hiding the feature if its deactivated. In the original issue, I mentioned also hiding the feature if its indirectly activated.

Thoughts?

@weihanglo weihanglo removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2022
@ehuss ehuss assigned epage and unassigned ehuss Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants