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

Switch arg parsing to clap v4 with derive #818

Merged
merged 1 commit into from
Mar 16, 2023

Conversation

tranzystorekk
Copy link
Contributor

Should make the code slightly more maintainable. Did my best to preserve old behavior, e.g. flags overriding themselves.

Notes:

  • clap 4 no longer colors the help prompt
  • clap 4 displays flags in help in the order they are defined in code
  • changed some value names to be more descriptive

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)

@codecov-commenter
Copy link

Codecov Report

Merging #818 (2d01c90) into master (ea56a7c) will increase coverage by 0.78%.
The diff coverage is 97.87%.

@@            Coverage Diff             @@
##           master     #818      +/-   ##
==========================================
+ Coverage   86.57%   87.35%   +0.78%     
==========================================
  Files          44       44              
  Lines        4507     4272     -235     
==========================================
- Hits         3902     3732     -170     
+ Misses        605      540      -65     
Impacted Files Coverage Δ
src/app.rs 39.28% <ø> (-46.19%) ⬇️
src/main.rs 29.41% <0.00%> (+3.09%) ⬆️
src/flags/blocks.rs 96.26% <96.29%> (+0.31%) ⬆️
src/flags/date.rs 96.32% <96.66%> (+2.66%) ⬆️
src/flags/recursion.rs 96.29% <97.56%> (+0.37%) ⬆️
src/display.rs 81.90% <100.00%> (ø)
src/flags.rs 96.77% <100.00%> (ø)
src/flags/color.rs 93.10% <100.00%> (+1.83%) ⬆️
src/flags/dereference.rs 100.00% <100.00%> (ø)
src/flags/display.rs 98.03% <100.00%> (-0.08%) ⬇️
... and 17 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zwpaper
Copy link
Member

zwpaper commented Mar 6, 2023

hi @tranzystorek-io, thanks so much for sending this big PR!

I have glanced at it mostly, and it lgtm in a glance. @meain what do you think about upgrading to v4, if you have not other concern we can play some attention on this and done the migration

@tranzystorekk
Copy link
Contributor Author

Heh, sorry for submitting a large chunk of changes like that, I got slightly carried away once I started drafting the code

@meain
Copy link
Member

meain commented Mar 16, 2023

@zwpaper If it looks good to you feel free you merge :D.

@zwpaper
Copy link
Member

zwpaper commented Mar 16, 2023

I have checked most of the code, it mostly looks good to me, and I will test it locally and merge if no problems occurred

@zwpaper zwpaper merged commit c267a04 into lsd-rs:master Mar 16, 2023
@tranzystorekk tranzystorekk deleted the clap-derive branch March 16, 2023 16:13
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.

4 participants