add ability to parse additional group methods #4688

klnusbaum · 2023-02-01T04:50:44Z

Towards completing #4574 , let's add the other methods for group derive.
This is my first PR for Clap and a lot of this was Cargo coding.
Please forgive anything that seems silly.
I'm very open to feedback :)

tests/derive/groups.rs

clap_derive/src/attr.rs

tests/derive/groups.rs

epage · 2023-02-01T17:20:49Z

tests/derive/groups.rs

+    use clap::CommandFactory;
+    let source_id = clap::Id::from("Source");
+    let dest_id = clap::Id::from("Dest");
+    let opt_command = Opt::command();
+    let source_group = opt_command
+        .get_groups()
+        .find(|g| g.get_id() == &source_id)
+        .unwrap();
+    let dest_group = opt_command
+        .get_groups()
+        .find(|g| g.get_id() == &dest_id)
+        .unwrap();
+    assert!(source_group.is_required_set());
+    assert!(dest_group.is_required_set());


Generally we do integration tests where we verify the behavior of the feature, like with assert_output

epage · 2023-02-01T17:20:57Z

Thanks for moving this forward!

klnusbaum · 2023-02-03T22:10:31Z

@epage I've made this more general, which I think addresses your comment about switching to a group_methods field on the Item struct. If this is the right direction, I can go ahead and beef up the tests.

clap_derive/src/derives/args.rs

epage · 2023-02-03T22:14:13Z

clap_derive/src/item.rs

+            match kind {
+                AttrKind::Group => self.group_methods.push(Method::new(name, quote!(#arg))),


Can you add a test where the group's id is overridden? I think that will break and instead overwrite the command's name.

Similarly, we need a ui-test ensuring that #[group(name = "foo")] fails as name isn't a group attribute

The ui-test exists but not the overridden id.

The problem I'm trying to highlight is that we are only setting group_methods in one branch and instead setting things on the Command in the other branches.

Maybe we should have a push_group_method function and make the switch between them higher up.

epage · 2023-02-03T22:14:43Z

If this is the right direction, I can go ahead and beef up the tests.

Yes it is, thanks!

This adds the "required" derive option for the group creation. Needed for clap-rs#4574.

klnusbaum · 2023-02-17T04:28:54Z

@epage I have a question I was hoping you might be able to help me with.

I can't figure out how to address the tests in tests/derive/groups.rs. The only way I can trigger them to run is cargo test --all. But I'd really like to be able to run just the single, new test I'm writing. Also, I'd like to be able to use cargo-expand to see what code is actually getting generated from my code changes.

I've never seen a dedicated tests folder like this before. Do you know how I can specifically refer to the groups tests?

epage · 2023-02-17T14:42:46Z

Normally tests/ are in individual files which makes them individual targets to be run like cargo test --test file -- testname. To reduce compilation overhead, we've merged them into larger test targets by using folders.

To run a specific test for derive, it would be cargo test -F derive --test derive -- testname. You can add the --exact flag if a similar test contains your tests name + more.

You would also run cargo expand --features derive --test derive. The file will be quite large but if you search in it for your test name, you can look at that and ignore the rest.

This adds the ability derive additional options for the group creation. Needed for clap-rs#4574.

epage

Thanks for continuing to move this forward!

epage · 2023-03-05T01:02:06Z

tests/derive/groups.rs

+        .find(|g| g.get_id() == &source_id)
+        .unwrap();
+    assert!(source_group.is_required_set());
+    // assert!(source_group.is_multiple()); currently broken. Fixed by PR #4704


Please make a note about this on the issue, not PR (as PRs come and go) and delete it.,

As-is, we have no way to track fixing this and it will get lost.

epage · 2023-03-05T01:05:38Z

tests/derive/groups.rs

+    let source_id = clap::Id::from("Source");
+    let opt_command = Opt::command();
+    let source_group = opt_command
+        .get_groups()
+        .find(|g| g.get_id() == &source_id)
+        .unwrap();
+    assert!(source_group.is_required_set());
+    // assert!(source_group.is_multiple()); currently broken. Fixed by PR #4704


We also tend to test the behavior from setting things rather than checking that they are set.

epage · 2023-03-05T01:07:55Z

clap_derive/src/item.rs

+            match kind {
+                AttrKind::Group => self.group_methods.push(Method::new(name, quote!(#arg))),


The ui-test exists but not the overridden id.

The problem I'm trying to highlight is that we are only setting group_methods in one branch and instead setting things on the Command in the other branches.

Maybe we should have a push_group_method function and make the switch between them higher up.

epage · 2023-03-27T22:25:24Z

FYI I've done some more work on this and closing this PR in favor of #4795

klnusbaum · 2023-04-03T20:43:39Z

@epage
This fell off my radar and I'm so sorry I didn't complete it in a timely manner. I really appreciate all the help you gave me trying to get this through. Thank you so much for everything and I'm super stoked to use the new stuff you got working.

Cheers :)

klnusbaum force-pushed the required_group branch 2 times, most recently from 2d3acf1 to 931eac5 Compare February 1, 2023 06:02

klnusbaum commented Feb 1, 2023

View reviewed changes

tests/derive/groups.rs Show resolved Hide resolved

epage reviewed Feb 1, 2023

View reviewed changes

clap_derive/src/attr.rs Outdated Show resolved Hide resolved

epage reviewed Feb 1, 2023

View reviewed changes

tests/derive/groups.rs Outdated Show resolved Hide resolved

epage reviewed Feb 1, 2023

View reviewed changes

klnusbaum changed the title ~~add "required" option for group attribute~~ add ability to parse additional group methods Feb 3, 2023

klnusbaum force-pushed the required_group branch from 840978f to 19f3b37 Compare February 3, 2023 22:09

klnusbaum commented Feb 3, 2023

View reviewed changes

clap_derive/src/derives/args.rs Show resolved Hide resolved

epage reviewed Feb 3, 2023

View reviewed changes

feat(derive): Add "required" option for groups

69fbc0f

This adds the "required" derive option for the group creation. Needed for clap-rs#4574.

feat(derive): Add additional group methods

f281c67

This adds the ability derive additional options for the group creation. Needed for clap-rs#4574.

klnusbaum force-pushed the required_group branch from 19f3b37 to f281c67 Compare March 4, 2023 21:13

epage reviewed Mar 5, 2023

View reviewed changes

epage mentioned this pull request Mar 27, 2023

feat(derive): Support #[group] attributes #4795

Merged

epage closed this Mar 27, 2023

klnusbaum deleted the required_group branch April 3, 2023 20:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ability to parse additional group methods #4688

add ability to parse additional group methods #4688

klnusbaum commented Feb 1, 2023 •

edited

epage Feb 1, 2023

epage commented Feb 1, 2023

klnusbaum commented Feb 3, 2023 •

edited

epage Feb 3, 2023

epage Mar 5, 2023

epage commented Feb 3, 2023

klnusbaum commented Feb 17, 2023

epage commented Feb 17, 2023 •

edited

epage left a comment

epage Mar 5, 2023

epage Mar 5, 2023

epage Mar 5, 2023

epage commented Mar 27, 2023

klnusbaum commented Apr 3, 2023

		match kind {
		AttrKind::Group => self.group_methods.push(Method::new(name, quote!(#arg))),

add ability to parse additional group methods #4688

add ability to parse additional group methods #4688

Conversation

klnusbaum commented Feb 1, 2023 • edited

epage Feb 1, 2023

Choose a reason for hiding this comment

epage commented Feb 1, 2023

klnusbaum commented Feb 3, 2023 • edited

epage Feb 3, 2023

Choose a reason for hiding this comment

epage Mar 5, 2023

Choose a reason for hiding this comment

epage commented Feb 3, 2023

klnusbaum commented Feb 17, 2023

epage commented Feb 17, 2023 • edited

epage left a comment

Choose a reason for hiding this comment

epage Mar 5, 2023

Choose a reason for hiding this comment

epage Mar 5, 2023

Choose a reason for hiding this comment

epage Mar 5, 2023

Choose a reason for hiding this comment

epage commented Mar 27, 2023

klnusbaum commented Apr 3, 2023

klnusbaum commented Feb 1, 2023 •

edited

klnusbaum commented Feb 3, 2023 •

edited

epage commented Feb 17, 2023 •

edited