Conversation
krzywon
left a comment
krzywon
left a comment
There was a problem hiding this comment.
See my inline comments for a few suggestions. For more global comments, I noted the first instance of that suggestion, but no others.
|
|
||
| from . import constants | ||
|
|
||
| Atom = Union["Element", "Isotope", "Ion"] |
There was a problem hiding this comment.
https://docs.python.org/3/library/typing.html#typing.Union says the shorthand "Element" | "Isotope" | "Ion" is recommended.
There was a problem hiding this comment.
These are strings. The or operator doesn't know how to turn them into types. This is fixed in the latest python (no need for quotes for as-yet-undefined types) but not yet in the supported older versions.
periodictable/activation.py
Outdated
|
|
||
| """ | ||
| fluence: float | ||
| CD_ratio: float |
There was a problem hiding this comment.
case mismatch between attr declaration and use CD_ratio vs Cd_ratio
bmaranville
left a comment
bmaranville
left a comment
There was a problem hiding this comment.
What is the overall goal of this PR? I still get 20 or more type errors when I load this code into my IDE with pyright - is there a type checker test that it now passes?
|
The goal is to improve type hinting for periodictable so that tab completion on attribute nams will work. Ideally it would be tested and correct, but I can't automate that without complicating the code. That will have to wait for a major release when I can simplify the API. Meanwhile, I will check the pyright errors one by one to see if there are any that can be addressed easily. |
3 participants
Adds support for type hinting which works in VScode. Fixes #104
Fixes the natural mass ratio calculation for compounds containing ionized isotopes. The old version was delegating from Ion to Isotope rather than Ion to Element for the natural density.
mypy still reports many errors. Most are from the use of None instead of NaN for undefined values. Type hinting and processing much more complicated to do correctly when you need to handle union of None and float. I'm not going to fix these are part of this PR.
There are warning about the assignment of properties to attributes which arise because of delayed loading of attributes. These are also out of scope.
My VSCode session feels laggy now that I have substantial type hinting coverage. We may need to trim back completeness in the interest of speed. Types like numpy ArrayLike require lots of inference.