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

Update clap #41

Merged
merged 21 commits into from
Nov 10, 2022
Merged

Update clap #41

merged 21 commits into from
Nov 10, 2022

Conversation

matthiasbeyer
Copy link
Contributor

This updates the clap dependency to clap 4.0.

This is a bit more involved, as clap changed quite a bit between major versions. Also, this introduces more typing to the CLI parsing, which should result in less validation/parsing code down the road, which is really nice.

There are some improvements I plan to make for the CLI parsing code that are not yet included here, but I guess as a first step this is more than enough. Feel free to review extensively. I think testing this would also be nice, especially since I cannot and as the parsing behaviour of clap might have changed in subtle ways that are not covered by any integration tests in this repository (would be really nice to have some...)

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Between clap 3.0 and clap 4.0, the way how values are fetched from the
CLI has changed. Notably the Arg::value_of and Arg::values_of functions
were replaced by something that can actually be a bit more typed.

This patch rewrites all these uses and introduces a bit more typing in
the CLI definition itself.

Types like integers or uuid::Uuid are moved to the CLI definition,
because these are trivial to make into clap ValueParser types.

More types should and will be transformed to be able to catch errors
already in CLI parsing instead of down the road during the execution of
butido.

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@matthiasbeyer matthiasbeyer force-pushed the update-clap branch 2 times, most recently from b0ae2eb to c1cdd3f Compare November 10, 2022 12:15
This rewrites the environment value parsing code using the previous
implementation of the CLI validator function, that had a more
sophisticated parsing implementation.

The CLI validator function gets then transformed to a value parser,
using that parsing code again.

Less code, less bugs. Yay!

Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@christophprokop
Copy link
Collaborator

Great! I tested with a few submits and arguments and it works great!
Merge right now or test some more? ;)

@matthiasbeyer
Copy link
Contributor Author

I'd say if your basic stuff works, go ahead and merge this!
If it turns out to have broken something we can always roll back 😉

@christophprokop
Copy link
Collaborator

move fast and break things? ;)
bors merge

@matthiasbeyer
Copy link
Contributor Author

move fast and break things? ;)

As long as CI is green, nothing is broken, right? 😉
We can actually take this as a check, ... if something breaks, we clearly need more testing in this repository! For example, integration tests are dearly missing here as well.

@christophprokop
Copy link
Collaborator

yeah, true :)

@bors
Copy link
Contributor

bors bot commented Nov 10, 2022

Build succeeded:

@bors bors bot merged commit 759353b into science-computing:master Nov 10, 2022
@matthiasbeyer matthiasbeyer deleted the update-clap branch November 10, 2022 14:22
@christophprokop
Copy link
Collaborator

christophprokop commented Nov 10, 2022

Of course right after merging i got this:

$ butido build tree -I local:rh8-default -L 1.8.0
error: Invalid value 'false' for '--env': Error during validation: 'Incomplete' is not a key-value pair

@matthiasbeyer matthiasbeyer mentioned this pull request Nov 10, 2022
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.

None yet

2 participants