-
Notifications
You must be signed in to change notification settings - Fork 192
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
Added support for queries against Materials Project #2097
Conversation
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.
Please address the in-line comments on the code. The rest of the code looks OK. It would be great if you could supply a couple of tests for the parts of the code that do not need the network access.
Yes, in fact I also wanted some tests for the actual pull, but then we need the key. A public test key supplied by MP would be great (e.g. just pull one demo structure for instance) but I do not know of such functionality at the moment. Right now I think the only sensible test is to test that an invalid key returns the right error message or something similar as that is already in |
I guess we're OK with tests that do not interact with the API for now. We might test the generation of Results and Entry instances. |
One more note on plugin naming: I suggest using full names ( |
Yes, but I just followed what was there already. Totally agree on this topic. I also believe this should be done in the code, regardless of clutter and longer lines. Changed this now, but will leave the old stuff for now. |
The importer seems to have disappeared from this PR... Could be due to rename? |
Should be fine now. Also added the |
Great! It's very close to be finished. I have noted a few minor changes, and then we are ready to 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.
Good to go. Many thanks!
Need to be able to pull data from the Materials Project. In this prototype, only the extraction of the structure is included. However, this can obviously be extended.