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 deps, including clap to v4 #235

Merged
merged 8 commits into from
Oct 13, 2022
Merged

Conversation

chevdor
Copy link
Contributor

@chevdor chevdor commented Oct 6, 2022

This PR makes #234 obsolete.

It bumps clap to v4. Clap v3 is now maintenance only.

Noticeable: for now using clap v4 means there is no more colored help. This is however likely to change in the future and there should be ways to reintroduce coloring if wanted.

NOTE: During the conversion from .takes_value(true) => num_args(...) I used a blanket range num_args(1..) which may be refined in some cases.

@bomgar
Copy link
Member

bomgar commented Oct 6, 2022

.num_args(0) does not work as expected for me. I did the migration brocode/fblog#65 just a few minutes ago and I had to change all the flags.

.action(ArgAction::SetTrue)

@chevdor
Copy link
Contributor Author

chevdor commented Oct 6, 2022

Yes, I was about to update with a DO NOT MERGE YET, I am spotting issues.

@chevdor
Copy link
Contributor Author

chevdor commented Oct 6, 2022

I fixed a bunch of issues but some probably remain and some testing is required.
I am switching my "default" to the binary produced here to see if I ran into problems.. but this is not exhaustive testing :)

@chevdor
Copy link
Contributor Author

chevdor commented Oct 6, 2022

The existing tests do pass though:

Compiling fw v2.16.1 (/Users/Shared/projects/fw)
    Finished test [unoptimized + debuginfo] target(s) in 1.45s
    Starting 18 tests across 1 binaries
        PASS [   0.032s] fw::bin/fw config::path::tests::test_do_expand_path
        PASS [   0.034s] fw::bin/fw config::tests::test_after_clone_from_tags_missing_all_tags_graceful
        PASS [   0.037s] fw::bin/fw config::path::tests::test_do_not_expand_path_without_tilde
        PASS [   0.038s] fw::bin/fw config::tests::test_after_clone_from_tags
        PASS [   0.037s] fw::bin/fw config::tests::test_after_clone_from_tags_prioritized
        PASS [   0.037s] fw::bin/fw config::tests::test_after_clone_override_from_project
        PASS [   0.037s] fw::bin/fw config::tests::test_workon_from_tags_missing_one_tag_graceful
        PASS [   0.041s] fw::bin/fw config::tests::test_workon_from_tags_missing_all_tags_graceful
        PASS [   0.042s] fw::bin/fw config::tests::test_workon_from_tags
        PASS [   0.045s] fw::bin/fw config::tests::test_after_clone_from_tags_missing_one_tag_graceful
        PASS [   0.055s] fw::bin/fw config::tests::test_workon_from_tags_prioritized
        PASS [   0.057s] fw::bin/fw git::tests::test_repo_name_from_ssh_pragma
        PASS [   0.056s] fw::bin/fw git::tests::test_repo_name_from_ssh_pragma_with_multiple_git_endings
        PASS [   0.058s] fw::bin/fw config::tests::test_workon_override_from_project
        PASS [   0.056s] fw::bin/fw git::tests::test_repo_name_from_url
        PASS [   0.058s] fw::bin/fw projectile::tests::test_replace_path_with_tilde
        PASS [   0.060s] fw::bin/fw projectile::tests::test_persists_projectile_config
        PASS [   0.067s] fw::bin/fw git::tests::test_username_from_git_url
------------
     Summary [   0.081s] 18 tests run: 18 passed, 0 skipped

@chevdor
Copy link
Contributor Author

chevdor commented Oct 6, 2022

I still run into issues, so this is not mergeable yet but I will need to jump on other duties.
I think you can commit into my branch, feel free to do so if you spot some of the issues.

src/app/mod.rs Outdated
.default_value("8")
.value_parser(clap::builder::RangedI64ValueParser::<i32>::new().range(0..=128))
.help("Sets the count of worker")
.takes_value(true),
.num_args(1..),
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a duplicate

src/app/mod.rs Outdated
@@ -136,7 +124,7 @@ For further information please have a look at our README https://github.com/broc
.help("Filter projects to import by state")
.long("include")
.short('a')
.takes_value(true)
.num_args(1..)
Copy link
Member

Choose a reason for hiding this comment

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

All occurrences of num_args(1..) including this one should be replaced by num_args(1) I guess.

@bomgar bomgar merged commit 76cca7f into brocode:master Oct 13, 2022
@bomgar
Copy link
Member

bomgar commented Oct 13, 2022

I merged this to master but I will not release it yet. I will test this for a while on my machine

@chevdor
Copy link
Contributor Author

chevdor commented Oct 13, 2022

Awesome, I will also install master locally and use it "in prod" then.

@chevdor chevdor deleted the wk-221006-clap4 branch October 13, 2022 12:36
@bomgar
Copy link
Member

bomgar commented Oct 13, 2022

I already had to push a few fixes :(

@chevdor
Copy link
Contributor Author

chevdor commented Oct 13, 2022

It does NOT look too good on my end: workon gives me an empty list.
EDIT: Juts seen your comment above 👌

@chevdor
Copy link
Contributor Author

chevdor commented Oct 13, 2022

I already had to push a few fixes :(

I rebuilt and indeed, your latest looks much better.

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

3 participants