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

Accept list[str] for thermo_types in ThermoRester.search() #729

Merged
merged 5 commits into from Jan 26, 2023

Conversation

janosh
Copy link
Member

@janosh janosh commented Jan 17, 2023

Let me know if I should add a test for this.

Also, I just used str in the type hint as using Literal would break single source of truth for accepted values. But happy to change that if you prefer Literal.

@janosh
Copy link
Member Author

janosh commented Jan 17, 2023

Am I causing this error?

>           raise MPRestError(
                f"REST query returned with error status code {response.status_code} "
                f"on URL {response.url} with message:\n{message}"
            )
E           mp_api.client.core.client.MPRestError: REST query returned with error status code 504 on URL https://api.materialsproject.org/materials/?deprecated=False&crystal_system=Cubic&_all_fields=True&_limit=1 with message:
E           Server timed out trying to obtain data. Try again with a smaller request.

@munrojm
Copy link
Member

munrojm commented Jan 17, 2023

Thank you for this @janosh, this looks great. The test failures aren't your fault. Something happened server-side so I am re-running. I'll merge once they all finish!

@munrojm munrojm added the release:patch Patch release label Jan 17, 2023
@janosh
Copy link
Member Author

janosh commented Jan 17, 2023

No problem. You prob know this but just in case not, you can enable auto-merge in the repo settings which will show a button below PRs to auto-merge as soon as CI passes.

@mattmcdermott
Copy link
Member

Just wanna give a +1 on merging this. I would much rather be passing a string :)

@mattmcdermott
Copy link
Member

Would also like to say that it would be nice if the available ThermoType values were added to the MPRester.get_entries and MPRester.get_entries_in_chemsys docstrings to make it easier for the user to configure what entries they want to access

@janosh
Copy link
Member Author

janosh commented Jan 25, 2023

@mattmcdermott You do get to see valid types from the error msg if you pass an invalid one. But I think you're right, they should also be in the doc string.

@munrojm munrojm merged commit 0713f8c into main Jan 26, 2023
@janosh janosh deleted the thermo-type-as-str branch January 26, 2023 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:patch Patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants