-
Notifications
You must be signed in to change notification settings - Fork 187
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
✨ NEW: structure.data.import
plugin entrypoint
#4427
✨ NEW: structure.data.import
plugin entrypoint
#4427
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4427 +/- ##
===========================================
+ Coverage 79.52% 79.54% +0.02%
===========================================
Files 519 519
Lines 37087 37088 +1
===========================================
+ Hits 29490 29497 +7
+ Misses 7597 7591 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
470f87c
to
03886a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @dev-zero - I see no issue with this and would support it (this would anyhow move outside aiida-core together with the materials-science specific classes, if that ever happens).
Could you please bring the PR up to date?
@chrisjsewell any comments?
03886a3
to
e7dbdd9
Compare
@ltalirz thanks a lot for the positive feedback, rebased as requested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisjsewell I approve this but I'd like one more pair of eyes on this.
Feel free to merge if you agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good cheers, but could you add a small bit of documentation of the feature around here thanks: https://github.com/aiidateam/aiida-core/blob/develop/docs/source/topics/plugins.rst#aiidacmdline
e7dbdd9
to
1793311
Compare
Done. Based on the already existing one with a spec, example and example usage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @dev-zero !
@chrisjsewell can you approve & merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cheers!
structure.data.import
plugin entrypoint
The use case is that I would like to add an option to import structures from a CP2K input file.
While this could be done in an
aiida-cp2k
cli or an extension ofverdi data
, it would make more sense to add it directly here.On the other hand it might not be a good idea to open up too many entry points since we would have to maintain those.