-
Notifications
You must be signed in to change notification settings - Fork 94
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
Type-checking maintenance updates #2016
Conversation
@@ -22,12 +22,11 @@ dependencies: | |||
- openff-interchange-base =0.4 | |||
- openff-nagl-base >=0.4.0 | |||
- openff-nagl-models >=0.3.0 | |||
- typing_extensions |
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.
This is a runtime requirement, not a "only when we run the type-checker" requirement. It's brought in by several other packages so it's not likely to be missed at runtime, but I'm listing it here according to the philosophy of listing all (non-optional) runtime requirements in this sections
and (chain.id == last_chain.id) | ||
and (chain.id == last_chain.id) # type: ignore[union-attr] |
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.
This appears to be the change that causes the breakage associated with #2014
@@ -76,6 +76,7 @@ exclude_lines = [ | |||
] | |||
|
|||
[tool.mypy] | |||
python_version=3.12 |
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.
If I'm reading things correctly, this setting allows for Mypy to "use" a particular version of Python no matter what is actually installed in the environment. I think this is a better setting to use than having the conditional live in the CI workflow because this (should!) ensure that all developers get a similar Mypy experience. I've been bit by something like this before
https://mypy.readthedocs.io/en/stable/config_file.html#confval-python_version
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.
Thanks @mattwthompson!
@@ -50,13 +49,3 @@ dependencies: | |||
- nbval | |||
# No idea why this is necessary, see https://github.com/openforcefield/openff-toolkit/pull/1821 | |||
- nomkl | |||
- mypy |
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.
(not blocking) just double checking that this removal is intended
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.
Yep - it's not run when this environment file is used
Resolves #2014