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

feat(derive): derive clap::Args for enum types #5700

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

feat(derive): derive clap::Args for enum types #5700

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f11570c
chore: REMOVE WHEN DONE: init flake
ysndr Aug 25, 2024
3ecaa53
test: Arg groups from enum
ysndr Aug 25, 2024
3249e4d
feat: Derive clap::Args for enums
ysndr Aug 25, 2024
75eb4ee
feat: Derive clap::FromArgMatches for enums
ysndr Aug 25, 2024
bd54e23
feat: Impl `update_from_arg_matches_mut` for enums
ysndr Aug 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions tests/derive/groups.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,3 +239,57 @@ For more information, try '--help'.
";
assert_output::<Opt>("test", OUTPUT, true);
}

#[test]
fn enum_groups_1() {
#[derive(Parser, Debug, PartialEq, Eq)]
struct Opt {
#[command(flatten)]
source: Source,
}

#[derive(clap::Args, Clone, Debug, PartialEq, Eq)]
enum Source {
A {
#[arg(short)]
a: bool,
#[arg(long)]
aaa: bool,
},
B {
#[arg(short)]
b: bool,
},
}

assert_eq!(
Opt {
source: Source::A {
a: true,
aaa: false,
}
},
Opt::try_parse_from(["test", "-a"]).unwrap()
);
assert_eq!(
Opt {
source: Source::A { a: true, aaa: true }
},
Opt::try_parse_from(["test", "-a", "--aaa"]).unwrap()
);
assert_eq!(
Opt {
source: Source::B { b: true }
},
Opt::try_parse_from(["test", "-b"]).unwrap()
);

assert_eq!(
clap::error::ErrorKind::ArgumentConflict,
Opt::try_parse_from(["test", "-b", "-a"])
.unwrap_err()
.kind(),
);
Comment on lines +287 to +292
Copy link
Member

Choose a reason for hiding this comment

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

When adding a test commit separate from a feature, it
needs to still pass tests / compile. I'm assuming this commit does neither.

Copy link
Author

Choose a reason for hiding this comment

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

How would i add a test that does compile for a thing that doesn't yet compile, or where the subject of the PR is allowing these constructs to compile in the first place?

Copy link
Member

Choose a reason for hiding this comment

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

"it depends"

Let's step back to the goals of why we encourage tests in a commit before the feature

  • Help test the tests (when fixing a bug, did the tests actually need to change?)
  • Through a commit's diff, making it clear to reviewers and the wider community what the intended behavior is

All of this is trivial when its a bug fix to help output. You show the existing behavior in the tests, then change the test when you change the behavior.

When the behavior doesn't exist yet, your options are

  • Just don't do this and have the test added in the commit.
  • Capture the feature failing spectacularly
  • Find the closest parallel feature and test that

The ideal is the last but sometimes there isn't always a close enough parallel and it can take the most work.

As an example of the last, take https://github.com/rust-lang/cargo/pull/14435/commits. This adds a new feature to Cargo. Originally, the test commit used the new features which didn't exist and errored out quickly. The commit that implemented the feature then made them not error. This was still more helpful than having them in the same commit because I only had to review the test output and not all of the test. However, I found a parallel feature and suggested it to the contributor. The tests are now much easier to see what the intended behavior is.

Thats easy when its all textual output and you aren't dealing with compile errors. One option is to write the tests with structs, rater than enums. The commit that adds the feature could also switch the structs to enums, causing some cases to still work while other cases will error.

That might or might not be worth it. If that seems like it will be too much or not show enough, feel free to say so, squash the commits, and move on.


// assert_eq!( )
}