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

Dissolve plugins #5554

Merged
merged 15 commits into from Apr 12, 2021
Merged

Dissolve plugins #5554

merged 15 commits into from Apr 12, 2021

Conversation

mih
Copy link
Member

@mih mih commented Apr 9, 2021

  • Converted all plugins into registered commands
  • Moved code and tests out of plugin and into local or distributed
  • Split up tests to have one file for each command
  • Adjust documentation
  • Adjusted logger names
  • Leave deprecation warnings and compatibility kludges
  • Try removing the plugin loading code

Fixes #4519

@codecov
Copy link

codecov bot commented Apr 9, 2021

Codecov Report

Merging #5554 (d1f66e9) into master (6d3ac7e) will decrease coverage by 11.89%.
The diff coverage is 80.74%.

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

@@             Coverage Diff             @@
##           master    #5554       +/-   ##
===========================================
- Coverage   90.17%   78.28%   -11.90%     
===========================================
  Files         299      305        +6     
  Lines       42528    42466       -62     
===========================================
- Hits        38351    33243     -5108     
- Misses       4177     9223     +5046     
Impacted Files Coverage Δ
datalad/api.py 67.39% <ø> (-8.48%) ⬇️
datalad/cmdline/tests/test_main.py 93.10% <ø> (-2.76%) ⬇️
...talad/distributed/tests/test_export_to_figshare.py 100.00% <ø> (ø)
datalad/interface/__init__.py 100.00% <ø> (ø)
datalad/local/tests/test_check_dates.py 44.68% <ø> (ø)
datalad/local/tests/test_export_archive.py 100.00% <ø> (ø)
datalad/plugin/__init__.py 0.00% <ø> (-90.00%) ⬇️
datalad/plugin/add_readme.py 0.00% <0.00%> (-93.11%) ⬇️
datalad/plugin/addurls.py 0.00% <0.00%> (-95.50%) ⬇️
datalad/plugin/check_dates.py 0.00% <0.00%> (-95.72%) ⬇️
... and 179 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 6d3ac7e...1418828. Read the comment docs.

@mih
Copy link
Member Author

mih commented Apr 9, 2021

For all ex-plugins datalad.tests.test_api.test_consistent_order_of_args fails. They don't have the "right" order -- and sure thing I still cannot infer what that order would be. The test already has traces of my struggle from earlier:

    elif intf.__name__ == 'ExtractMetadata':
        # MIH I was never sure what this test enforces and it takes
        # me ages each time to try to wrap my head around it
        # I am confident that I do not want to change the API of this
        # command now that it is a command and no longer a plugin
        # hence this exception
        eq_(spec_posargs, spec_posargs)

What ever these commands where doing, it was wrong in some way. However, I think I will just extend the list of exceptions -- I doubt someone prefers to break API for sudden compliance with a rule whose violation has never been a problem.

@mih
Copy link
Member Author

mih commented Apr 9, 2021

I have to detach from this project now. Feel free to have at it

@bpoldrack
Copy link
Member

bpoldrack commented Apr 9, 2021

What ever these commands where doing, it was wrong in some way. However, I think I will just extend the list of exceptions -- I doubt someone prefers to break API for sudden compliance with a rule whose violation has never been a problem.

I think it's about the order in the _params_ dict only (positionals first). So - no API breakage needed to comply. However, I don't think it has any actual effect (anymore?), not even on building the docs.

IIRC at some point it played a role when binding to Dataset. Probably just obsolete. Do you recall anything else, @yarikoptic ?

@mih
Copy link
Member Author

mih commented Apr 9, 2021

I think it's about the order in the _params_ dict only (positionals first).

Hu? dict has no order.

@bpoldrack
Copy link
Member

Hu? dict has no order.

It does. Didn't we have that topic recently? Since 3.6 or something.

But I misread the code nevertheless, so ... what the test does seem to try to ensure is, that positional arguments in python interface are first in signature and match what's declared positional for CLI in that dict.

@mih
Copy link
Member Author

mih commented Apr 9, 2021

Hu? dict has no order.

It does. Didn't we have that topic recently? Since 3.6 or something.

But I misread the code nevertheless, so ... what the test does seem to try to ensure is, that positional arguments in python interface are first in signature and match what's declared positional for CLI in that dict.

Ha, true! So, any necessary change (which I am still not sure what it needs to be) would change the API. If that is true, I maintain my attitude and approach.

@bpoldrack
Copy link
Member

bpoldrack commented Apr 9, 2021

Ha, true! So, any necessary change (which I am still not sure what it needs to be) would change the API. If that is true, I maintain my attitude and approach.

Yeah - I'm trying to decide whether to remove the test or make it more clear, since I'm struggled with that several times, too.

Example: AddReadme: dataset is first in python signature and positional - this seems wrong indeed. Moving it to first position for binding is done by the datasetmethod decorator - no need for that in __call__'s signature. But that way dataset is mandatory in unbound call, while filename becomes a kw arg in python but is positional in CLI. Compliance and consitency could be achieved by mainting CLI and changing python API (slightly). Not sure, what to make of that really, but seems to me it actually is an annoyance, that simply didn't spring to the eye due to a lack of examples pointing out, how the different possible calls in python are confusing and do not as directly translate in a CL call like other commands do.

Edit:
Atleast in that example I think I'm for a change, @mih. Make dataset a kw arg and swap position with filename. Doesn't change CLI, doesn't really change bound call, but makes an unbound call consistent with the other forms and with other commandsd (where dataset is an optional kw arg).

@kyleam
Copy link
Contributor

kyleam commented Apr 9, 2021

CrippledFS failure looks like gh-5300.

@mih
Copy link
Member Author

mih commented Apr 10, 2021

@bpoldrack If you have a vision re API changes, go for it. I consider this a change separate from this PR, which only aims to remove the infrastructure for plugins. The commands are only touched insofar as they still relied on that infrastructure.

@bpoldrack
Copy link
Member

I consider this a change separate from this PR

Agreed. Moved into two issues.

@mih mih added corpse-in-basement merge-if-ok OP considers this work done, and requests review/merge labels Apr 12, 2021
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.

Looks good to me. Thanks for doing the tedious conversion.

@mih
Copy link
Member Author

mih commented Apr 12, 2021

Thx @kyleam for the review and the fix!

@mih mih merged commit f937558 into datalad:master Apr 12, 2021
@mih mih deleted the rf-wtf branch April 12, 2021 15:11
kyleam added a commit to kyleam/datalad that referenced this pull request Apr 12, 2021
Calling get_interface_groups() with include_plugins=True was marked as
deprecated in dataladgh-5554.
kyleam added a commit to kyleam/datalad that referenced this pull request Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
corpse-in-basement 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.

Dissolve plugins (and supporting functionality)
3 participants