-
Notifications
You must be signed in to change notification settings - Fork 8
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
Pluginise mpes reader #243
Conversation
ca90db9
to
bb168ef
Compare
Pull Request Test Coverage Report for Build 7977723693Details
💛 - Coveralls |
@sherjeelshabih I removed all the mpes related tests and move this into the reader plugin. Do you think we need to recreate some tests for the example reader to cover functionality? @rettigl This is mainly fyi. When everything is moved you need to do an additional |
That's a good idea also for the other plugins. Maybe we could also make an install extra as [all-readers] or something that installs all plugins (at least the ones that we are aware of or for which we provide its own install extra)? EDIT: I think in general it would be nice to also list the supported plugins in the pynxtools main repo somewhere (maybe also in README?) |
Yes, this is a good idea. We should have this for all plugins we support and I think it's also fine if people want to add their readers there.
Yes, maybe we can get the list from the pynxtools-plugin topic. That way it would automatically update :) If not we can just keep a list but link to the topic. |
I'll have a look on what tests etc will be needed separately. You can keep this PR for dealing with the MPES reader. I'll make an issue to check test coverage again for the JOSS paper. |
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.
LGTM, will do the same for https://github.com/FAIRmat-NFDI/pynxtools-xps.
I tested it, and it works, however this changes the functionality of pynxtools, so we should discuss how to adjust versioning accordingly (we are probably the only ones using the mpes reader right now, but the next version of pynxtools without extras definition will break the behavior). |
Yes, we should follow proper semantic versioning for pynxtools to indicate when we introduce breaking changes. See #225. Ideally, we would now do the main changes until a v1.0.0 release so that we don't have to introduce breaking changes for a long time. I would do a v0.1.0 release for the next version to indicate that we break something. |
The MPES reader is moving into its own plugin at https://github.com/FAIRmat-NFDI/pynxtools-mpes
Fixes #234