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.

Sign up for GitHub

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

Jump to bottom

Allow groups of groups #5711

Open
epage opened this issue Aug 30, 2024 · 3 comments
Open

Allow groups of groups #5711

epage opened this issue Aug 30, 2024 · 3 comments
Labels
C-enhancement Category: Raise on the bar on expectations E-medium Call for participation: Experience needed to fix: Medium / intermediate

Comments

@epage
Copy link
Member

epage commented Aug 30, 2024

A blocker for #4697 which is important for #3123 and #2621 is the ability to have a group own another group.

API-wise, I suspect we'd change ArgGroup::arg to ArgGroup::member or ArgGroup::child to generalize it for taking either an arg or a group ID.

  • To meet our compatibility goals,. this "change" would be a deprecation and a new function.
    • I'd recommend doing this with a PR that (1) adds the new function, (2) updates existing uses to the new function, (3) adds the deprecation notice
    • We might need to soft-deprecate it and not apply the attribute because a true deprecation must be done on a point release

To unblock this, we'd need to update the debug assertions to make sure the Id is a valid Arg or Group Id.

There is also the validation work
@epage epage added C-enhancement Category: Raise on the bar on expectations E-medium Call for participation: Experience needed to fix: Medium / intermediate labels Aug 30, 2024
@epage epage mentioned this issue Aug 30, 2024
Open
@ysndr
Copy link

ysndr commented Sep 4, 2024 •
edited
Loading

I've been looking into this a bit more.

Are you sure there are still "debug assertions to make sure the Id is a valid Arg or Group Id" in the project?

Grepping for debug_assert didn't yield anything looking close to that.
The assertions i found are apparently only checking for Id but don;t assert what that Id represents: [1].
Also it seems to check arg in group.args against both arg_ids and group_ids: [2]
That is to say, I dont think "groups of groups" are particularly disallowed.

Variable naming is a bit biased, "arg" is everywhere around those methods even if it is explicitly checked against group_ids as well.

Renaming the method(s) on ArgGroup is relatively straight forward (sans all the doc tests..), i figure this should actually go further a bit, also changing "arg" at the usage side to "member".
Since i'm particularly interested in the conflict side tho i might work on that first before hunting down umpteen "arg" variables that should be "member" instead.

[1] https://github.com/ysndr/clap/blob/6539ab81c4858e051dfd2d76a984faec75c7b88d/clap_builder/src/builder/debug_asserts.rs#L307-L327

[2] https://github.com/ysndr/clap/blob/6539ab81c4858e051dfd2d76a984faec75c7b88d/clap_builder/src/builder/debug_asserts.rs#L290-L305

Edit:
Turns out the handling for groups in [1] were prior changes of mine that snuck into the renaming commit.

@epage
Copy link
Member Author

epage commented Sep 4, 2024

I'm a bit confused. You pointed at code in debug_asserts.rs in 6539ab8 to show that child groups are checked but for 6539ab8 github is reporting "This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.".

If I look in

clap/clap_builder/src/builder/debug_asserts.rs

Lines 290 to 299 in 64e3790

for arg in &group.args {
// Args listed inside groups should exist
assert!(
cmd.get_arguments().any(|x| x.get_id() == arg),
"Command {}: Argument group '{}' contains non-existent argument '{}'",
cmd.get_name(),
group.get_id(),
arg
);
}

which is the latest commit for master
https://github.com/clap-rs/clap/blob/master/clap_builder/src/builder/debug_asserts.rs#L290-L299
that code is not there.

This matches the behavior you saw in #5700 (comment)

@ysndr
Copy link

ysndr commented Sep 4, 2024

Oh my that happens when you start off with existing work and mess up your rebase.

So those lines 290 - 300 were my changes from investigating the original subgroups approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
C-enhancement Category: Raise on the bar on expectations E-medium Call for participation: Experience needed to fix: Medium / intermediate
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@epage @ysndr