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

Revert to getopts for argument parsing #169

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Scandiravian
Copy link
Contributor

Changes

  • Revert to getopts for argument parsing
  • Make cli backwards compatible with previous versions

Related Issues

I'd like to get some input on the trajectory of my work to revert back to getopts, so I can hopefully get it to an acceptable state in the next couple of days. Please let me know if there's any concerns or comments about the work so far 🙂

I'm not entirely happy with the changes yet, as I think the main() function has become a bit too complex. There's also a .to_string call in the create_hook_files that I'd like to get rid of.

@codecov
Copy link

codecov bot commented Oct 17, 2021

Codecov Report

Merging #169 (21c7e96) into master (f431408) will decrease coverage by 16.16%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##            master     #169       +/-   ##
============================================
- Coverage   100.00%   83.83%   -16.17%     
============================================
  Files            4        4               
  Lines          142      167       +25     
============================================
- Hits           142      140        -2     
- Misses           0       27       +27     
Impacted Files Coverage Δ
src/git.rs 64.28% <ø> (-35.72%) ⬇️
src/rusty_hook.rs 73.58% <0.00%> (-26.42%) ⬇️
src/hooks.rs 97.14% <100.00%> (-2.86%) ⬇️
src/config.rs 89.23% <0.00%> (-10.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f431408...21c7e96. Read the comment docs.

@calebcartwright
Copy link
Member

Haven't really had a chance to go through this yet, but in case it's helpful you can take a look at what we had prior to the original switch from getopts to clap in https://github.com/swellaby/rusty-hook/blob/39fbca3b90c6f37bfdacd23bc4d25b9a0da554f0/src/main.rs

The only potential user interaction with the executable is rusty-hook init, and even that won't happen in cases where folks use the "easy button" approach of adding rusty-hook as a dev dependency. As such, I'm not super concerned with providing an extremely rich user experience; rusty-hook is primarily executed by git hook scripts users never see nor care about

@calebcartwright
Copy link
Member

Apologies, I thought CI was failing outright here, but see it's coverage related. That's a somewhat surprising drop, do you think it's legitimate based on the diff or a specious tarpaulin and/or codecov issue?

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.

CLI changes for v0.12.0 not compatible with all earlier scripts
2 participants