Remove TopologyAtom from docstrings, add some annotations
#1584
Conversation
mattwthompson
commented
Apr 18, 2023
•
mattwthompson
commented
- Tag issue being addressed
- Add tests
- Update docstrings/documentation, if applicable
- Lint codebase
- Update changelog
| def atom(self, atom_topology_index): | ||
| def atom(self, atom_topology_index: int) -> "Atom": |
There was a problem hiding this comment.
This is the main change I care about. The current code has this unfortunate downstream behavior:
from openff.toolkit import Molecule
topology = Molecule.from_smiles("CCO").to_topology()
reveal_type(topology.atom(0).atomic_number)$ mypy j.py
j.py:5: note: Revealed type is "Any"
Success: no issues found in 1 source fileAlone this is not the end of the world but Atom is often a launching point for much more information. The case I ran into was a type checker thinking that
atomic_numbers: tuple[int, ...] = tuple(
a.atomic_number for a in interchange.topology.atoms
)was actually tuple[Any]
| raise AtomNotInTopologyError( | ||
| f"No atom with index {atom_topology_index} exists in this topology, " | ||
| f"which contains {self.n_atoms} atoms." | ||
| ) |
There was a problem hiding this comment.
This function's logic has an expression after the return statement, which confused the type checker. I threw this in not because it should ever be hit, but maybe it's possible.
|
I see this isn't marked "Ready for review" but IMO this is mergeable in its current state (only other thing I could think of is releasenotes, but that's not a blocker). Please merge when you're ready! |
|
My oversight - I was unsure if the added exception block was too ugly to get through, but that's the opposite of not asking for feedback 🙃 |
|
Thanks for the review - and allowing me to do something ugly in the advancement of non-ugly things |
