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

Avoid using clap deprecated features #787

Merged
merged 25 commits into from
Jan 12, 2023

Conversation

sudame
Copy link
Contributor

@sudame sudame commented Jan 11, 2023

The PR will fix #786 .

πŸ’ͺ Motivation

In order to minimize future security risks and maintain ease of use, it is recommended to avoid using deprecated features of clap.

βœ… What I did

I have refactored the code so that no warning appears when the following command is executed:

cargo check --features clap/deprecated

This command was introduced for migration to v4, but we can use it here.

πŸ”„ Main Changes

also see πŸ‘€ : https://github.com/clap-rs/clap/blob/master/CHANGELOG.md

in app.rs ...

  • App -> Command
  • use Arg::action
    • use ArgAction::Append instead of Arg::take_value
    • use ArgAction::SetTrue for bool flags (like lsd -a)
  • Arg::validator -> Arg::value_parser
    • I changed the validator's type and it returns Ok(validated_value).
    • πŸ“ related Doc (use Fn(&str) -> Result<T, E> pattern.)

in flags/*.rs ...

  • Command::get_matches_from_safe -> Command::try_get_matches_from
  • ArgMatches::is_present -> ArgMatches::get_one::<bool>
  • ArgMatches::occurrences_of(id) > 0 -> ArgMatches::value_source(id) == ValueSource::CommandLine
    • ArgMatches::occurrences_of(id) returns the number of times an argument was used at runtime, so we can replace it with ValueSource::CommandLine check.
    • πŸ“ : related CHANGELOG
  • ArgMatches::values_of -> ArgMatches::get_many::<T>
  • ArgMatches::value_of -> ArgMatches::get_one::<T>
    • In many place I used ArgMatches::get_*::<String> and then map(String::as_str) to conform to existing codes which uses &str.

πŸ’­ Comments

  • There are a lot of commits because of my habits. Please inform me if it is too many.

TODO

  • Use cargo fmt
  • Add necessary tests
  • Add changelog entry
  • Update default config/theme in README (if applicable)
  • Update man page at lsd/doc/lsd.md (if applicable)

@sudame sudame requested review from meain and Peltoche as code owners January 11, 2023 09:06
@zwpaper
Copy link
Member

zwpaper commented Jan 12, 2023

Thanks so much for this detailed PR @sudame!

there is one more thing that may need you to do, there are too many tiny commits, please squash your commits.

@meain
Copy link
Member

meain commented Jan 12, 2023

@zwpaper if it just about squashing commits, we can do that on Github itself if everything else looks good.
2023-01-12-09-49-36

@zwpaper
Copy link
Member

zwpaper commented Jan 12, 2023

I had checked the PR and have no questions, thanks so much for this serious PR! @sudame

@zwpaper zwpaper merged commit da61909 into lsd-rs:master Jan 12, 2023
@sudame sudame deleted the fix/avoid-using-clap-deprecated-features branch January 12, 2023 06:10
@sudame
Copy link
Contributor Author

sudame commented Jan 12, 2023

Thank you for the prompt response β€οΈπŸš€

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.

lsd uses many deprecated features of clap
3 participants