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

Major CLI refactoring #6378

Merged
merged 2 commits into from
Feb 1, 2022
Merged

Major CLI refactoring #6378

merged 2 commits into from
Feb 1, 2022

Conversation

mih
Copy link
Member

@mih mih commented Jan 26, 2022

This PR aims to replace the previous CLI with a consolidated code base. Detailed history of the changes since c666b36 is available at https://github.com/datalad/datalad-cli

Initial benchmark estimates:

          305±2ms          297±3ms     0.98  cli.startup.time_command_execution
          290±3ms          300±5ms     1.03  cli.startup.time_command_help_np
          289±6ms          297±5ms     1.03  cli.startup.time_command_short_help
-      1.84±0.03s       1.51±0.02s     0.82  cli.startup.time_help_np
-      1.87±0.03s       1.49±0.01s     0.80  cli.startup.time_short_help
          276±6ms          280±3ms     1.01  cli.startup.time_usage_advice

No major slowdown, despite simplified implementations in some places.
Global help generation sped up, by avoiding to parse extensions twice.

TODO:

  • ~80% coverage of datalad.cli from datalad.cli.tests alone
  • Replace datalad.cli internal import locations for datalad API components
  • Provide a design document
  • Build API docs for datalad.interface.base
  • Inspect the diff for oddities resulting from the enormous code shift
  • deprecated extension still uses deprecated from datalad.cmdline.helpers import get_repo_instance, which is removed here (maybe needs to come back, was only deprecated with 0.16)
  • Replace all remaining datalad.cmdline imports

Changelog

💫 Enhancements and new features

  • The command line interface help-reporting has been sped up by ~20% #6370 (by @mih)

🪓 Deprecations and removals

  • All code in module datalad.cmdline was (re)moved, only datalad.cmdline.helpers.get_repo_instanceis kept for a deprecation period (by @mih)

📝 Documentation

  • API docs for datalad.interface.base are now included in the documentation (by @mih)
  • A new design document is provided that describes the basics of the command line interface implementation #6382 (by @mih)

🏠 Internal

  • The command line interface was refactored and consolidated into a new, more self-contained datalad.cli module #6383 (by @mih)

🛡 Tests

  • The new datalad.cli.tests have an improved module coverage of 80% (by @mih)

@mih mih added the do not merge Not to be merged, will trigger WIP app "not passed" status label Jan 26, 2022
@mih mih force-pushed the nocmdline branch 3 times, most recently from e8db678 to 089daa8 Compare January 27, 2022 15:39
@mih mih added semver-minor Increment the minor version when merged team-core core API/commands (https://github.com/datalad/datalad/issues/6365) labels Jan 27, 2022
@mih mih changed the title Demo: CLI code Major CLI refactoring Jan 27, 2022
@mih mih added the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Jan 28, 2022
@mih mih force-pushed the nocmdline branch 5 times, most recently from d1fc89a to 6c018d8 Compare January 28, 2022 08:24
@mih mih marked this pull request as ready for review January 28, 2022 11:04
@codecov
Copy link

codecov bot commented Jan 28, 2022

Codecov Report

Merging #6378 (dcb8704) into master (9aa3a8c) will decrease coverage by 0.38%.
The diff coverage is 82.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6378      +/-   ##
==========================================
- Coverage   89.84%   89.46%   -0.39%     
==========================================
  Files         331      342      +11     
  Lines       43155    42822     -333     
==========================================
- Hits        38773    38309     -464     
- Misses       4382     4513     +131     
Impacted Files Coverage Δ
datalad/cmdline/helpers.py 30.43% <ø> (-43.64%) ⬇️
datalad/interface/__init__.py 100.00% <ø> (ø)
datalad/interface/base.py 90.00% <ø> (+2.46%) ⬆️
datalad/interface/tests/test_base.py 96.55% <ø> (-1.81%) ⬇️
datalad/interface/tests/test_docs.py 100.00% <ø> (ø)
datalad/runner/exception.py 100.00% <ø> (ø)
datalad/tests/test_utils.py 97.05% <ø> (-0.49%) ⬇️
datalad/tests/utils.py 87.38% <ø> (-2.03%) ⬇️
datalad/utils.py 82.19% <ø> (-3.07%) ⬇️
datalad/cli/tests/test_formatters.py 17.77% <55.55%> (ø)
... and 68 more

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 9aa3a8c...dcb8704. Read the comment docs.

@mih mih removed do not merge Not to be merged, will trigger WIP app "not passed" status CHANGELOG-missing When a PR's description does not contain a changelog item, yet. labels Jan 29, 2022
@codeclimate
Copy link

codeclimate bot commented Jan 29, 2022

Code Climate has analyzed commit dcb8704 and detected 77 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 18
Security 15
Bug Risk 16
Style 28

View more on Code Climate.

@mih mih mentioned this pull request Jan 29, 2022
@mih
Copy link
Member Author

mih commented Feb 1, 2022

Ok, no objections were voiced. Some additional fixes will be coming in via #6391

@mih mih merged commit c2183b3 into datalad:master Feb 1, 2022
@mih mih deleted the nocmdline branch February 1, 2022 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor Increment the minor version when merged team-core core API/commands (https://github.com/datalad/datalad/issues/6365)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Major RF of the command line interface Provide design doc on the CLI import timing
1 participant