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

Make --no-default-groups work with --all-groups #10891

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

j178
Copy link
Contributor

@j178 j178 commented Jan 23, 2025

Summary

Closes #10890

Comment on lines 2713 to 2714
#[arg(long, conflicts_with_all = ["only_group", "all_groups"])]
pub group: Vec<GroupName>,
Copy link
Member

@zanieb zanieb Jan 23, 2025

Choose a reason for hiding this comment

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

I don't think these need to conflict, they're just redundant? (--group foo --all-groups)

Comment on lines -2725 to 2726
#[arg(long, conflicts_with_all = ["no_group", "only_group"])]
#[arg(long, conflicts_with_all = ["only_group"])]
pub no_default_groups: bool,
Copy link
Member

Choose a reason for hiding this comment

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

👍 yeah this was wrong

Similarly, I think --only-group is just redundant? --only-group foo --no-default-groups doesn't need to error.

Copy link
Contributor Author

@j178 j178 Jan 24, 2025

Choose a reason for hiding this comment

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

The current flag conflicts in groups are inconsistent (for example, --only-dev should conflict with --group, but it doesn't). I'd like to address these in a separate PR.

Comment on lines 2733 to 2734
#[arg(long, conflicts_with_all = ["group", "no_default_groups", "all_groups"])]
pub only_group: Vec<GroupName>,
Copy link
Member

Choose a reason for hiding this comment

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

Agree this should conflict with --all-groups

@j178 j178 force-pushed the groups-cli branch 2 times, most recently from 38c263e to 26f70b0 Compare January 23, 2025 16:02
@j178 j178 changed the title Make --no-default-groups works with --all-groups Make --no-default-groups work with --all-groups Jan 24, 2025
DevMode::Exclude => false,
DevMode::Include | DevMode::Only => group == &*DEV_DEPENDENCIES,
}
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is not used anywhere.

}
}

/// Returns `true` if the specification includes the given group.
pub fn contains(&self, group: &GroupName) -> bool {
pub fn contains(&self, group: &GroupName, is_default_group: bool) -> bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The is_default_group: bool argument is not ideal, but it seems to be the simplest solution I could come up with.

@j178 j178 marked this pull request as ready for review January 24, 2025 08:50
@Gankra Gankra self-requested a review January 24, 2025 15:56
@charliermarsh charliermarsh self-assigned this Jan 25, 2025
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.

--no-default-groups should be able to use with --no-group and --all-groups
3 participants