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.

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

Upgrade clap and clap_complete version #154

Merged
merged 7 commits into from Mar 28, 2023
Merged

Upgrade clap and clap_complete version #154

merged 7 commits into from Mar 28, 2023

Conversation

TheRustyPickle
Copy link
Contributor

The PR contains all the changes required for the cli to work with the latest clap version. I also took the liberty to upgrade clap_complete. Resolves #152

Comment

test_get_start_and_uds_client_command had to be removed because clap::Command no longer implements PartialEq, Eq. If you have any other suggestions, let me know.

Suggestion

  • clap::error has render().ansi() function now. Enabling it can do this: image1 to image 2
  • I also noticed here that in some cases because of both print and the Err() return, the error message gets printed twice. Remove the print and only return the Err() here?

Thank you, @24seconds!

@24seconds 24seconds self-requested a review March 20, 2023 23:36
@24seconds 24seconds added the dependencies Pull requests that update a dependency file label Mar 20, 2023
@24seconds
Copy link
Owner

[About the suggestions]

  1. Good. Nice!
  2. Good! Thank you for catching this!

@TheRustyPickle
Copy link
Contributor Author

I just added both suggestions that I mentioned. Also realized, due to the shell completion approach, it is possible to support all 5 shells that clap_complete support so add all 5 of them. Let me know your thoughts.

@24seconds
Copy link
Owner

24seconds commented Mar 23, 2023 •
edited

[About the comment]
I looked the code to understand the purpose of test_get_start_and_uds_client_command.
This test exists to check that get_start_and_uds_client_command should contain exact subcommands - what get_common_subcommands provides. This is because preventing the discrepancy between what get_main_command provides and what get_start_and_uds_client_command provides when the new subcommand is introduced.

Therefore, I think we should write some test code to check this. Comparing command was the previous implementation and we now need a new way to check this.

@24seconds
Copy link
Owner

I will be back on weekend 🙇

@TheRustyPickle
Copy link
Contributor Author

Let me know what you think of the updated test.

24seconds
24seconds reviewed Mar 27, 2023
View reviewed changes
Copy link
Owner

@24seconds 24seconds left a comment

Choose a reason for hiding this comment

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

Sorry for the late 🙇.
Looks good overall. I have a question about takes_value. Could you take a look?

src/error.rs Show resolved Hide resolved
src/command/util.rs Outdated Show resolved Hide resolved
src/command/handler/user_input.rs Show resolved Hide resolved
src/command/application.rs

let args: Vec<&Arg> = command
Copy link
Owner

Choose a reason for hiding this comment

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

[Comment]
Good. Maybe we should move this test to another test case. Let's do this later.

src/command/application.rs Show resolved Hide resolved
src/command/action.rs Show resolved Hide resolved
@TheRustyPickle
Copy link
Contributor Author

Suggestion: Command::new("completion") should be moved inside get_start_and_uds_client_command as the completion command does not work when running the cli app.

@24seconds
Copy link
Owner

Suggestion: Command::new("completion") should be moved inside get_start_and_uds_client_command as the completion command does not work when running the cli app.

@TheRustyPickle good. Thank you for caring. When you are ready, please utilize 're-request' review feature in github 🙇‍♂️

24seconds
24seconds approved these changes Mar 28, 2023
View reviewed changes
Copy link
Owner

@24seconds 24seconds left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you so much!!! 🚀

src/command/application.rs
.subcommands({
let mut cmd = get_common_subcommands();
cmd.push(
Command::new("completion")
Copy link
Owner

Choose a reason for hiding this comment

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

[comment]
Let's make completion string as enum if it is possible. For example, ActionType::Completion.

src/command/application.rs

command
.long("default")
.action(ArgAction::SetTrue),
Copy link
Owner

Choose a reason for hiding this comment

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

Good

@24seconds 24seconds merged commit 7e53410 into 24seconds:main Mar 28, 2023
2 checks passed
@24seconds 24seconds mentioned this pull request Mar 28, 2023
Merged
@TheRustyPickle TheRustyPickle deleted the clap-upgrade branch March 28, 2023 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@24seconds 24seconds 24seconds approved these changes

Assignees

@TheRustyPickle TheRustyPickle

Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dependency upgrade: clap version (3.2.22 -> 4.1.10)
2 participants
@TheRustyPickle @24seconds