-
Notifications
You must be signed in to change notification settings - Fork 862
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
Guard MSONAtoms
definition behind ASE package availability
#3645
Conversation
that 👍 |
As opposed to a linter rule that only catches my fix ;)
|
9d43e87
to
0424e72
Compare
Makes sense. Thanks! |
I'm actually a bit confused why |
If anyone is interested, click details below... I guess the reason is that Found 788 errors in 81 files (checked 279 source files)
|
types and we're running i realized i never bothered to check if https://github.com/RobertCraigie/pyright-python re comment, pretty sure
|
on a related note, the args: ["--ignore-missing-imports", "--scripts-are-modules"] |
Could we get this in then? I don't know how quickly you'd want to make another release but this does break several other packages at the moment (even just within the MP ecosystem -- atomate2, emmet, mp_api etc). Any fixing or new linting rules could be tried out in #3646 or otherwise handled by the maintainers as they see fit going forward |
2ed483c
to
ef43267
Compare
i'll take a look at this tomorrow morning. it'll definitely be in the next release. we need one for several other issues as well |
…vert MSONAtoms implementation
…whole test_ase.py module
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.
Scratch that, misread it! LGTM |
i hope not. the behavior is meant to be unchanged. given this class only raises on init, i don't see how you'd get an error on import?
|
You just beat my ninja edit, in the time it took me to walk to my computer to test it out - never review PRs from your phone! Thanks for updating this @janosh |
Summary
Hopefully closes #3644. Not sure if this is the approach you want to take.
Todo
It would be nice to add a test or linter rule that would have caught the initial issue.