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

Annotate most everything #1798

Merged
merged 22 commits into from Mar 21, 2024
Merged

Annotate most everything #1798

merged 22 commits into from Mar 21, 2024

Conversation

mattwthompson
Copy link
Member

@mattwthompson mattwthompson commented Dec 20, 2023

Happy holidays @Yoshanuikabundi - I think I'll break this up into a few smaller changes but this is the gist of what is encompassed

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Merging #1798 (45c7876) into main (b739698) will decrease coverage by 0.02%.
The diff coverage is 95.92%.

Additional details and impacted files

@mattwthompson mattwthompson marked this pull request as ready for review January 3, 2024 13:15
Copy link
Collaborator

@Yoshanuikabundi Yoshanuikabundi left a comment

Choose a reason for hiding this comment

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

This is awesome Matt, great work. I think I only found 1 mistake where there was some vestigial type annotation in a docstring, and all my other comments are pretty much just ways to get rid of type: ignore comments that you can adopt if you like. Sorry this took so long to review and thanks so much for doing it!

openff/toolkit/topology/_mm_molecule.py Show resolved Hide resolved
openff/toolkit/topology/_mm_molecule.py Outdated Show resolved Hide resolved
openff/toolkit/topology/_mm_molecule.py Outdated Show resolved Hide resolved
openff/toolkit/topology/molecule.py Outdated Show resolved Hide resolved
openff/toolkit/topology/molecule.py Outdated Show resolved Hide resolved
Comment on lines 3303 to 3304
assume_missing_is_default
???
Copy link
Collaborator

Choose a reason for hiding this comment

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

This argument isn't used in the function body, so I guess it's ignored and just here to avoid a breaking change? If so we should document that it doesn't do anything or remove it.

@j-wags Any idea what this parameter is supposed to do?

Copy link
Member Author

Choose a reason for hiding this comment

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

openff/toolkit/utils/collections.py Outdated Show resolved Hide resolved
openff/toolkit/utils/collections.py Outdated Show resolved Hide resolved
openff/toolkit/utils/openeye_wrapper.py Outdated Show resolved Hide resolved
openff/toolkit/utils/rdkit_wrapper.py Outdated Show resolved Hide resolved
@mattwthompson
Copy link
Member Author

Thanks for the review @Yoshanuikabundi - there are three loose ends I'm leaving unresolved, but I think everything else here is working!

@mattwthompson mattwthompson merged commit 858d55e into main Mar 21, 2024
18 checks passed
@mattwthompson mattwthompson deleted the shift-annotations branch March 21, 2024 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move type annotations from docstrings to signatures wherever possible
2 participants