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

Add alias .to_file() for .to() method of structures and molecules #3356

Merged
merged 9 commits into from Sep 30, 2023

Conversation

QuantumChemist
Copy link
Contributor

@QuantumChemist QuantumChemist commented Sep 27, 2023

Summary

I have noticed that many beginners (including myself a while ago) find it very unintuitive to write e.g. structures to a file by using .to(), especially with the .from_file() method counterpart. That's why I added aliases to_file for structures and molecules.

PS: This is the second attempt of #3354 as something went wrong there...

@shyuep
Copy link
Member

shyuep commented Sep 27, 2023

You don't actually need to write it this way. All that needs to be done is to add to_file = to and write_file = to in SiteCollection. That will create the aliases without so much code.

@QuantumChemist
Copy link
Contributor Author

QuantumChemist commented Sep 27, 2023

You don't actually need to write it this way. All that needs to be done is to add to_file = to and write_file = to in SiteCollection. That will create the aliases without so much code.

I know, that's how I started, but when I tried to call the alias in pycharm, it wouldn't give me the input hints and at least for me they are really important. If you think that's not necessary, then I can change it to the simple alias.

Also when I only added to_file = to in SiteCollection, it would throw an error saying something like "cannot call abstract method" (I'm writing this from my phone, I would need to check the details tomorrow again if you wanna know the exact error)

@janosh janosh added needs testing PRs that are not ready to merge due to lacking tests api Application programming interface labels Sep 28, 2023
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

We definitely need a test for the new alias.

@QuantumChemist
Copy link
Contributor Author

We definitely need a test for the new alias.

Aye aye! 🫡

@QuantumChemist
Copy link
Contributor Author

hi @janosh I hope the tests are ok like that?

Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

Thanks @QuantumChemist! 👍

@janosh janosh changed the title Adding alias .to_file() for .to() method of structures and molecules Add alias .to_file() for .to() method of structures and molecules Sep 30, 2023
@janosh janosh enabled auto-merge (squash) September 30, 2023 17:16
@janosh janosh merged commit b246e56 into materialsproject:master Sep 30, 2023
1 check passed
@janosh janosh added ux User experience and removed needs testing PRs that are not ready to merge due to lacking tests labels Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Application programming interface ux User experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants