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

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
63 changes: 63 additions & 0 deletions flake.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 36 additions & 0 deletions flake.nix
Copy link
Author

Choose a reason for hiding this comment

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

don't mind this it just helps me get a rust compiler using nix...
will be removed before this is ready to go anywhere

Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
{
inputs.fenix = {
url = "github:nix-community/fenix";
inputs.nixpkgs.follows = "nixpkgs";
};

outputs =
{ nixpkgs, fenix, ... }:
let
pkgs = nixpkgs.legacyPackages.aarch64-darwin;
in
{
devShells.aarch64-darwin.default = pkgs.mkShell {
shellHook = ''
export RUST_SRC_PATH="${pkgs.rustPlatform.rustLibSrc}";
'';
packages = [
pkgs.lldb
pkgs.libiconv
pkgs.darwin.apple_sdk.frameworks.Security
pkgs.darwin.apple_sdk.frameworks.SystemConfiguration
(fenix.packages.aarch64-darwin.combine [
fenix.packages.aarch64-darwin.stable.cargo
fenix.packages.aarch64-darwin.stable.rust
fenix.packages.aarch64-darwin.targets.wasm32-wasi.stable.rust-std
fenix.packages.aarch64-darwin.targets.wasm32-wasip1.stable.rust-std
fenix.packages.aarch64-darwin.targets.aarch64-apple-darwin.stable.rust-std
])
];
};

formatter.aarch64-darwin = pkgs.nixfmt-rfc-style;

};

}
54 changes: 54 additions & 0 deletions tests/derive/groups.rs
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!( )
}