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

CLI: click -> argparse #830

Merged
merged 9 commits into from
Oct 9, 2022
Merged

CLI: click -> argparse #830

merged 9 commits into from
Oct 9, 2022

Conversation

tony
Copy link
Member

@tony tony commented Oct 2, 2022

  • Remove click and sphinx-click
  • Add sphinx-argparse
  • Use argparse
  • Remove completion, for now

See also: vcs-python/vcspull#400

@codecov
Copy link

codecov bot commented Oct 2, 2022

Codecov Report

Merging #830 (afb7af3) into master (0a6e23a) will decrease coverage by 0.42%.
The diff coverage is 71.77%.

@@            Coverage Diff             @@
##           master     #830      +/-   ##
==========================================
- Coverage   75.45%   75.03%   -0.43%     
==========================================
  Files          19       19              
  Lines        1434     1602     +168     
  Branches      331      359      +28     
==========================================
+ Hits         1082     1202     +120     
- Misses        265      290      +25     
- Partials       87      110      +23     
Impacted Files Coverage Δ
docs/conf.py 58.94% <ø> (ø)
src/tmuxp/cli/utils.py 63.50% <58.40%> (-15.67%) ⬇️
src/tmuxp/cli/load.py 67.34% <70.14%> (+1.42%) ⬆️
src/tmuxp/cli/freeze.py 71.27% <72.22%> (-0.16%) ⬇️
src/tmuxp/cli/convert.py 82.35% <77.27%> (+0.21%) ⬆️
src/tmuxp/cli/edit.py 76.92% <80.00%> (+6.92%) ⬆️
src/tmuxp/cli/import_config.py 79.36% <84.21%> (+19.90%) ⬆️
src/tmuxp/cli/shell.py 85.45% <87.80%> (-3.12%) ⬇️
conftest.py 87.50% <100.00%> (ø)
src/tmuxp/cli/debug_info.py 100.00% <100.00%> (ø)
... and 5 more

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

@tony tony force-pushed the vanilla-argparse branch 2 times, most recently from 9e71c62 to 30e5183 Compare October 4, 2022 02:02
@piotr-dobrogost
Copy link

Why are you replacing click with argparse?

@tony
Copy link
Member Author

tony commented Oct 5, 2022

@piotr-dobrogost

  • click:
    • Pros:
      • Decorators
      • Completions
      • Testing is ok
    • Cons:
      • Extra dependency

        • This can cause headaches downstream on linux packages
      • Completion cumbersome in various ways (context sharing, testing, lack of documentation)

      • Passing contexts is cumbersome and sometimes buggy

      • Click's documentation

        Basic patterns and context passing aren't covered

      • Our docs: sphinx-click is hard to work with / control

        It automatically creates a section that's the exact name of the header that the user would want to write by hand

  • argparse
    • Pros:
      • Well maintained

      • New python versions are getting new features (3.9 and 3.10, when our minimum version is bumped, automatically gets these)

      • Works out of the box

      • Our docs: sphinx-argparse allows very fine-grained control

      • Has third party completion tools

        argcomplete, shtab

      • Easy for downstream package maintainers

      • Easy to test

    • Cons:
      • Very manual (this could also be considered a plus)
      • Parser creation and running happens in a separate function for each command
      • Completion is a separate package - so we end up needing a new package anyway - but it can be optional

@tony tony force-pushed the vanilla-argparse branch 17 times, most recently from b8279e5 to c0c9fdc Compare October 9, 2022 22:39
@tony tony merged commit 9813ce2 into master Oct 9, 2022
@tony tony deleted the vanilla-argparse branch October 9, 2022 23:01
tony added a commit that referenced this pull request Oct 9, 2022
tony added a commit that referenced this pull request Oct 9, 2022
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

2 participants