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

BF: cmdline: Restore argcomplete functionality #4477

Merged
merged 1 commit into from May 3, 2020

Conversation

kyleam
Copy link
Contributor

@kyleam kyleam commented May 1, 2020

The optimization in fb38c70 (OPT/RF: create only the subparser
needed for the command, offer "closest" choices, 2018-05-17) sped up
datalad, but it (would have [*]) broken the argcomplete-based
command-line completion due to the spots in setup_parser() that raise
an exception rather than returning the parser that argcomplete needs
to inspect.

To make completion work again, add a flag to setup_parser() that
signals that datalad is being called by argcomplete. In this
scenario, avoid failing early so that argcomplete can do its thing.

Closes #4473.

[*]: When I tried from fb38c70^, completion still didn't work
because an exception is raised in config.py due to cfg_kv_regex
not matching when datalad is run under argcomplete (for reasons I
haven't looked into). But, when I worked around that error,
then fb38c70 broke completion. Note that the cfg_kv_regex
matching has since changed (b733b04, 2020-04-17) in a way that
avoids the config.py exception.

The optimization in fb38c70 (OPT/RF: create only the subparser
needed for the command, offer "closest" choices, 2018-05-17) sped up
datalad, but it (would have [*]) broken the argcomplete-based
command-line completion due to the spots in setup_parser() that raise
an exception rather than returning the parser that argcomplete needs
to inspect.

To make completion work again, add a flag to setup_parser() that
signals that datalad is being called by argcomplete.  In this
scenario, avoid failing early so that argcomplete can do its thing.

Closes datalad#4473.

[*]: When I tried from fb38c70^, completion still didn't work
     because an exception is raised in config.py due to cfg_kv_regex
     not matching when datalad is run under argcomplete (for reasons I
     haven't looked into).  But, when I worked around that error,
     _then_ fb38c70 broke completion.  Note that the cfg_kv_regex
     matching has since changed (b733b04, 2020-04-17) in a way that
     avoids the config.py exception.
@codecov
Copy link

codecov bot commented May 1, 2020

Codecov Report

Merging #4477 into maint will decrease coverage by 0.00%.
The diff coverage is 60.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #4477      +/-   ##
==========================================
- Coverage   89.68%   89.68%   -0.01%     
==========================================
  Files         275      275              
  Lines       37102    37103       +1     
==========================================
  Hits        33275    33275              
- Misses       3827     3828       +1     
Impacted Files Coverage Δ
datalad/cmdline/main.py 77.08% <60.00%> (-0.33%) ⬇️

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 13045c8...d9dd7d6. Read the comment docs.

@yarikoptic
Copy link
Member

Awesome, thank you @kyleam! I wondered if it would be feasible to craft a test (not necessarily in this PR, but while the track is warm) to sure that argcomplete is happy with us?

@kyleam
Copy link
Contributor Author

kyleam commented May 1, 2020

I wondered if it would be feasible to craft a test (not necessarily in this PR, but while the track is warm) to sure that argcomplete is happy with us?

Sure, I'll make an issue and assign it to you so you don't forget ;)

@yarikoptic
Copy link
Member

Ha ha, my track is cold! But I guess I could try by setting that env variable and setting what is spit out

Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for me! Awesome! Thx!

@mih
Copy link
Member

mih commented May 3, 2020

I hope you'll sort out who will provide the test first. Meanwhile I will merge this and autocomplete all day. Thx much!

@mih mih merged commit efdd9a8 into datalad:maint May 3, 2020
@mih
Copy link
Member

mih commented May 3, 2020

Resolved conflict with master, merged, and pushed.

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

3 participants