Skip to content

Use clap instead of getopts to parse cli args #27

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
Merged
merged 14 commits into from
Jun 28, 2022
Merged

Use clap instead of getopts to parse cli args #27

merged 14 commits into from
Jun 28, 2022

Conversation

wert007
Copy link
Owner

@wert007 wert007 commented Jun 9, 2022

clap allows using subcommands, which are helpfull for the input
methods. This also makes the main method a lot shorter, since there is
no need to add every single command line option per manually. This is
handled by the derive feature of clap and the new Args struct.

commit message

I open this pull request to further discuss things and ensure feature equality, before merging it into main.

Pinging @kevinmatthes

@kevinmatthes
Copy link
Contributor

Would you please post the link to the original repository of clap here? I would like to a look on the implemented traits since I experimented with getopts on how the getopts::Options and their getopts::Matches can be stored in structs to thin out the main.

The main problem was that getopts::Options does not implement neither Copy nor Clone, so storing the options in a struct is somehow difficult with the getopts crate. But with the getopts::Matches, everything is alright.

@wert007
Copy link
Owner Author

wert007 commented Jun 9, 2022

https://github.com/clap-rs/clap

The nice thing here are two things. The first one is, that the cli args are defined in the rust code itself and are turned into the cli at compile time. This is way shorter then declaring every single option with a function call or something similar. The other being, that the input methods are turned into subcommands, which means, that no check is necessary, if it is specified, or if there are multiple specified.

kevinmatthes
kevinmatthes reviewed Jun 10, 2022
View reviewed changes
kevinmatthes
kevinmatthes reviewed Jun 10, 2022
View reviewed changes
@wert007 wert007 force-pushed the feature/clap branch 2 times, most recently from 5446e14 to bb27308 Compare June 11, 2022 08:47
kevinmatthes
kevinmatthes suggested changes Jun 11, 2022
View reviewed changes
Copy link
Contributor

@kevinmatthes kevinmatthes left a comment

Choose a reason for hiding this comment

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

Thank you for your recent changes! I left some comments regarding further enhancements.

@kevinmatthes kevinmatthes mentioned this pull request Jun 11, 2022
Merged
kevinmatthes
kevinmatthes suggested changes Jun 12, 2022
View reviewed changes
Copy link
Contributor

@kevinmatthes kevinmatthes left a comment

Choose a reason for hiding this comment

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

Thank you for your latest changes! I left some feedback for you!

kevinmatthes
kevinmatthes reviewed Jun 13, 2022
View reviewed changes
@kevinmatthes
Copy link
Contributor

By the way: I took a look at the slidesv3 project you referenced above. It might be implemented even "rustier" and more portable with a justfile instead of all these Windows scripts. The justfile is not considered a script at all but triggers the Makefile syntax highlighting of VS Code-like IDEs (as you seem to utilise as of your .gitignore).

Further readings:

wert007 added 3 commits June 18, 2022 11:27
@wert007
Use clap instead of getopts to parse cli args
41eab69 
`clap` allows using subcommands, which are helpfull for the input
methods. This also makes the main method a lot shorter, since there is
no need to add every single command line option per manually. This is
handled by the `derive` feature of `clap` and the new `Args` struct.
@wert007
Turn public fields of Args into getter
f236a2d
@wert007
Improve Args documentation by implementing feedback
c88c1df
@wert007
Copy link
Owner Author

wert007 commented Jun 18, 2022

By the way: I took a look at the slidesv3 project you referenced above. It might be implemented even "rustier" and more portable with a justfile instead of all these Windows scripts. The justfile is not considered a script at all but triggers the Makefile syntax highlighting of VS Code-like IDEs (as you seem to utilise as of your .gitignore).

Further readings:

While this might be true, I have multiple reasons against that. For one this project is currently only developed on windows. It is also not really up to collaboration, so it does not really need to be portable at the time being. Also requiring just would add a not really necessary dependency, which I'm not really a fan of.

kevinmatthes
kevinmatthes suggested changes Jun 19, 2022
View reviewed changes
Copy link
Contributor

@kevinmatthes kevinmatthes left a comment

Choose a reason for hiding this comment

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

There are still some small language issues.

kevinmatthes
kevinmatthes reviewed Jun 22, 2022
View reviewed changes
@kevinmatthes
Copy link
Contributor

You forgot numerous docstrings which still carry Imperative / Present. Since you oversaw them multiple times, I would like to suggest to revert the Indicative / Simple Present. It does not add anything to the source files.

kevinmatthes
kevinmatthes suggested changes Jun 22, 2022
View reviewed changes
Copy link
Contributor

@kevinmatthes kevinmatthes left a comment

Choose a reason for hiding this comment

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

Here are still some open points I would to ask you to change.

wert007 added 5 commits June 24, 2022 12:28
@wert007
Improve Args documentation by implementing feedback
2ceb4c5
@wert007
Rename field verbose in Args
49cedc2 
Renames field verbose in Args to better fit getter is_verbose.
@wert007
Reorder struct Args to the beginning
3d7227c 
Also moves related structs and enums closer to Args as well.
@wert007
Create Filter by reference to Args
1e2619c 
This makes the main function a lot easier, since Args is not moved into
Filter anymore.
@wert007
Run cargo fmt and cargo clippy
69c563c
wert007 and others added 2 commits June 24, 2022 12:45
@wert007
Fix documentation and field order
b3021c5 
This adds/improves documentation in `Args` and fixes the documentation
in other places. Also this orders the fields of `Args` so that the
`Filter` related fields come last, since they are the least important,
and take up a lot of space.
@kevinmatthes @wert007
[Enhancement] Fix #25; enhance Cargo.toml
59a6b74
@wert007 wert007 merged commit ea628c2 into main Jun 28, 2022
@wert007 wert007 deleted the feature/clap branch June 28, 2022 20:17
@kevinmatthes kevinmatthes mentioned this pull request Jun 29, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinmatthes kevinmatthes kevinmatthes requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wert007 @kevinmatthes