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

Migrate to clap 4 #87

Merged
merged 8 commits into from
Feb 1, 2023
Merged

Migrate to clap 4 #87

merged 8 commits into from
Feb 1, 2023

Conversation

marcospb19
Copy link
Collaborator

@marcospb19 marcospb19 commented Jan 13, 2023

This PR migrates from clap 2 to clap 4, and does a little
bit of refactoring on the code that it touches.

Fixes #86.

It also adds -m as a short flag for --maxsize.

@jyn514 jyn514 self-assigned this Jan 13, 2023
Copy link
Collaborator

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This is amazing! Thank you for putting the move from main.rs to cli.rs in its own commit! It made this much easier to review :)

I have a lot of unimportant nits and one blocking comment about --version.

src/main.rs Outdated Show resolved Hide resolved
src/cli.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
src/cli.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
tests/usage.txt Show resolved Hide resolved
tests/usage.txt Show resolved Hide resolved
src/cli.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Collaborator

jyn514 commented Jan 13, 2023

Ah, I missed the last two commits - I would prefer to put those in a follow-up PR if possible, the clap migration is already quite large.

src/main.rs Outdated Show resolved Hide resolved
@marcospb19
Copy link
Collaborator Author

I'll be separating the last two commits for the next PR. 👍

@marcospb19
Copy link
Collaborator Author

This is amazing! Thank you for putting the move from main.rs to cli.rs in its own commit! It made this much easier to review :)

I wanted to do it in even smaller pieces, but the migration commit had to do a lot of stuff at the same time.

I unindented most of the main function by one level, the diff is terribly unreadable because of it, so here's a tip, if you disable whitespace diffing:

image

You'll have 60 fewer modifications to review in that commit 😉.

now when running it like `cargo sweep`, the --version is available,
which is necessary for when cargo-sweep is ran via cargo, also, --help
shows the crate description (from TOML manifest)
by considering that files can be outputted in different order
@marcospb19 marcospb19 marked this pull request as ready for review January 30, 2023 06:25
@marcospb19 marcospb19 marked this pull request as draft January 30, 2023 06:46
@marcospb19 marcospb19 requested a review from jyn514 January 30, 2023 06:46
Copy link
Collaborator

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Happy to merge this with the last 2 "take criterion as ..." commits dropped :)

@marcospb19 marcospb19 requested a review from jyn514 January 31, 2023 23:42
@marcospb19 marcospb19 marked this pull request as ready for review January 31, 2023 23:42
@jyn514 jyn514 merged commit a36602f into holmgr:master Feb 1, 2023
@marcospb19 marcospb19 deleted the migrate-to-clap-4 branch February 1, 2023 01:57
@marcospb19 marcospb19 mentioned this pull request Sep 12, 2023
@marcospb19 marcospb19 mentioned this pull request Oct 5, 2023
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.

Show toolchain list only once when the -i/--installed flag is provided
2 participants