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.

Sign up for GitHub

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

Jump to bottom

Add data plugin command entry point #993

Merged
merged 10 commits into from
Dec 20, 2017
Merged

Add data plugin command entry point #993

merged 10 commits into from
Dec 20, 2017

Conversation

DropD
Copy link
Contributor

@DropD DropD commented Dec 14, 2017

close #992

@DropD DropD added priority/important topic/data-types topic/plugin-system topic/verdi type/accepted feature approved feature request good first issue Issues that should be relatively easy to fix also for beginning contributors labels Dec 14, 2017
@DropD DropD added this to the 1.0 release milestone Dec 14, 2017
@DropD DropD requested a review from ltalirz December 14, 2017 13:33
@ltalirz
Copy link
Member

ltalirz commented Dec 14, 2017

@DropD very much looking forward to this one!
Let me know when it passes the test.

@DropD
Copy link
Contributor Author

DropD commented Dec 14, 2017

@ltalirz Tests are passing now, it was only a missing requirement.

ltalirz
ltalirz suggested changes Dec 14, 2017
View reviewed changes
Copy link
Member

@ltalirz ltalirz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks fine to me, I'm just struggling to get it to work. How do I need to decorate my cli function now such that it is recognised?
I tried a few things in the aiida-plugin-template, but I didn't manage to get it to work (data plugin didn't show up in verdi data).

Please provide a brief example or documentation.

@DropD
Copy link
Contributor Author

DropD commented Dec 18, 2017

I will add documentation with links to click docs and an example.

@DropD DropD removed the good first issue Issues that should be relatively easy to fix also for beginning contributors label Dec 18, 2017
@DropD DropD force-pushed the add-data-plugin-cmd branch from 908c925 to 34f9eb5 Compare December 19, 2017 12:14
@DropD
Copy link
Contributor Author

DropD commented Dec 19, 2017

@ltalirz please check this out, build the documentation and try to use Developers guide > Developer commandline plugin tutorial.

@DropD DropD self-assigned this Dec 19, 2017
ltalirz
ltalirz reviewed Dec 19, 2017
View reviewed changes
docs/source/developer_guide/devel_tutorial/cmdline_plugin.rst

Append to file ``float_cmd.py``::

@float_cmd.command()
Copy link
Member

Choose a reason for hiding this comment

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

Don't you need @float_cmd.command('export') here?

Copy link
Contributor Author

@DropD DropD Dec 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Nope, the command decorator uses the function name by default. I will add a paragraph explaining this. The changes are added in 7cfd00a.

docs/source/developer_guide/devel_tutorial/cmdline_plugin.rst Outdated

The discovery of plugins via entry points follows exactly the same mechanisms as all other plugin types.

The possibility of pluging cli commands into each other is a feature of ``click`` a python library that greatly simplifies the task. You can find in-debth documentation here: `Click 6.0 docs <click_docs>`_.
Copy link
Member

Choose a reason for hiding this comment

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

plugging, in-depth

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 7cfd00a.

docs/source/developer_guide/devel_tutorial/cmdline_plugin.rst Outdated
Append to file ``float_cmd.py``::

@float_cmd.command()
@click.option('--outfile', '-o', type=click.Path(dir_ok=False), help='write output to this file (by default print to stout).'
Copy link
Member

Choose a reason for hiding this comment

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

should be dir_okay

Copy link
Contributor Author

@DropD DropD Dec 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed. Fixed in 7cfd00a.

@DropD DropD force-pushed the add-data-plugin-cmd branch from 34f9eb5 to 7cfd00a Compare December 19, 2017 16:48
@DropD
Copy link
Contributor Author

DropD commented Dec 19, 2017

The documentation was also updated for the tutorial parts to line up in the side bar. To mark them better visually, all their titles now start with 'Tutorial:'. The old data plugin command part was marked as outdated with a .. warning::. And should be removed eventually.
It might however be better to reimplement the examples in the new system first?

@ltalirz
Copy link
Member

ltalirz commented Dec 19, 2017

Marking the tutorials in the developer documentation more clearly is a good first step.

In my opinion, further reorganisation is badly needed - the aiida developer documentation contains an enormous amount of information that should be categorised (and quite a bit of which is outdated).
@giovannipizzi I'm happy to prepare some thoughts on this. Then, perhaps the two of us can at some point take 1-2 hours to go through the whole developer documentation together to agree on how to move things (+what to delete). After this, I can implement them.

@giovannipizzi giovannipizzi merged commit e172e62 into aiidateam:develop Dec 20, 2017
@giovannipizzi
Copy link
Member

@ltalirz ok!

@sphuber sphuber mentioned this pull request Feb 3, 2018
Closed
@sphuber sphuber modified the milestones: 1.0 release, v1.0.0 May 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ltalirz ltalirz ltalirz approved these changes

Assignees

@DropD DropD

Labels
priority/important topic/data-types topic/plugin-system topic/verdi type/accepted feature approved feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add entry point for plugin data commands
4 participants
@DropD @ltalirz @giovannipizzi @sphuber