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

ENH: datalad shell-completion helper #5544

Merged
merged 4 commits into from Apr 16, 2021

Conversation

yarikoptic
Copy link
Member

This PR continues the streak of "silly" UX ones (following #5543)

Well, we have tools/cmdline-completion helper which nobody sees and
which we do not distribute via pypi, conda-forge, etc.
To make it easy to enable completion on any installation (yet to try
on Windows), this command could provide easy to access way.

As this is a new minor command, duplicating what we shipped under tools/ already, positioned it against maint.

@codecov
Copy link

codecov bot commented Apr 3, 2021

Codecov Report

Merging #5544 (abcd79e) into maint (b01be3f) will decrease coverage by 5.41%.
The diff coverage is 100.00%.

❗ Current head abcd79e differs from pull request most recent head df83f88. Consider uploading reports for the commit df83f88 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##            maint    #5544      +/-   ##
==========================================
- Coverage   90.19%   84.77%   -5.42%     
==========================================
  Files         296      295       -1     
  Lines       42055    42081      +26     
==========================================
- Hits        37930    35673    -2257     
- Misses       4125     6408    +2283     
Impacted Files Coverage Δ
datalad/interface/__init__.py 100.00% <ø> (ø)
datalad/core/distributed/tests/test_clone.py 90.46% <100.00%> (-6.90%) ⬇️
datalad/interface/run_procedure.py 93.29% <100.00%> (+0.04%) ⬆️
datalad/interface/shell_completion.py 100.00% <100.00%> (ø)
datalad/interface/tests/test_run_procedure.py 100.00% <100.00%> (ø)
datalad/interface/tests/test_shell_completion.py 100.00% <100.00%> (ø)
datalad/version.py 40.90% <100.00%> (ø)
datalad/support/tests/utils.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/metadata/extractors/tests/test_audio.py 19.35% <0.00%> (-80.65%) ⬇️
datalad/metadata/extractors/xmp.py 12.96% <0.00%> (-79.63%) ⬇️
... and 89 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 b01be3f...df83f88. Read the comment docs.

Well, we have tools/cmdline-completion helper which nobody sees and
which we do not distribute via pypi, conda-forge, etc.
To make it easy to enable completion on any installation (yet to try
on Windows), this command could provide easy to access way.
@yarikoptic yarikoptic added the merge-if-ok OP considers this work done, and requests review/merge label Apr 3, 2021
@bpoldrack
Copy link
Member

Fine with me as a way to make it more discoverable.

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.

Good idea!

datalad/interface/base.py Outdated Show resolved Hide resolved
datalad/interface/shell_completion.py Outdated Show resolved Hide resolved
yarikoptic and others added 2 commits April 7, 2021 15:39
Co-authored-by: Michael Hanke <michael.hanke@gmail.com>
Making shell-completion into a full featured new type of interface to enjoy all
the levels of decoration for results evaluation, so we could even call it
as

    dl.shell_completion(result_xfm='relpaths')

To be appreciated while listening to https://www.youtube.com/watch?v=u9Dg-g7t2l4
yarikoptic added a commit to yarikoptic/datalad that referenced this pull request Apr 15, 2021
…LDSTYLE_COMMANDS

ATM it is needed to define _no_eval_results attribute to signal docbuilder to
not populate docs for all the additional kwargs provided by @eval_results.
Some commands even acquired @eval_results (e.g. create_sibling) but still had
that attribute still set to True for unknown reason (probably just an effect of
the decentralized logic during RFing).  The effect of this was not
mentioned probably because outside users/developers  do not care to look up for
those possible additional kwargs, and "core" developers simply know about them.

I believe a similar situation was with common options defined at top level for
datalad (--on-error, etc) -- since old interfaces did not care to return/yield
structured records, they could (and still can) be provided but have no real
effect on anything.  So even though I had to tune up some tests in
interfaces/tests/test_base.py I expect them to be of no negative effect.

This change ENHs @eval_results to add _eval_results attriute to the wrapper, so
outside logic could consistently decide to either add docstrings AND to handle
all those additional kwargs.  Even though we would still have some common
options which are not applicable to interfaces which have no @eval_results, I
expect situation to not differ pragmatically in that regard.

As a result it should no longer require listing of OLDSTYLE commands.

I can see how some developers might take it as undesired effect since now there
would be no hardcoded listing of commands to RF or get rid of.  Since we
define a single Interface per file, analogous check could be done e.g.  via

	$> git grep -l 'class.*(Interface)' | xargs grep -L @eval_results | grep -v -e '/test_'
	datalad/distribution/create_sibling_github.py
	datalad/distribution/create_test_dataset.py
	datalad/interface/add_archive_content.py
	datalad/interface/ls.py
	datalad/interface/test.py
	datalad/support/sshrun.py

in the core codebase (so we would indeed loose some hits from extensions).

But to me, and with shell-completion
(datalad#5544) and sshrun helpers as use cases,
it seems more of a matter of the fact that not all Interfaces should provide
fancy results handling: they might be leaf interfaces not intended for nesting
in a larger logic, not operate on datasets or be generators, have no associated
with their action paths, etc.  I really see no reason to complicate their code
with more code to accomplish the same mission and possibly only to complicate
debugging.  I think we should allow for those to co-exist in peace with
fancy @eval_result'ed interfaces.

If we decide to allow for "simple" interfaces (for such helpers as above), we
might want to RF further to derive an InterfaceWithResults to avoid ad-hoc
_has_eval_results_call, which this PR introduces, and/or to avoid explicit
addition of the @eval_results for every __call__ of every InterfaceWithResults.
The above witch hunt also could be carried through a simple grep
'class.*(Interface)'.
@yarikoptic
Copy link
Member Author

d;oh -- need to add some minimal test...

Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

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

It seems all comments are addressed, and it looks good to me.

@kyleam kyleam merged commit 1a36272 into datalad:maint Apr 16, 2021
@yarikoptic yarikoptic deleted the enh-shell-completion branch April 29, 2021 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok OP considers this work done, and requests review/merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants