Skip to content

docs: type hinting and docstrings in molstruct#497

Merged
DaniBodor merged 4 commits intomainfrom
molstruct_type_hints
Sep 13, 2023
Merged

docs: type hinting and docstrings in molstruct#497
DaniBodor merged 4 commits intomainfrom
molstruct_type_hints

Conversation

@DaniBodor
Copy link
Copy Markdown
Collaborator

This was really bugging me while refactoring query, so did a separate extra PR for it.

@DaniBodor DaniBodor requested a review from gcroci2 September 12, 2023 15:25
@DaniBodor DaniBodor mentioned this pull request Sep 12, 2023
Copy link
Copy Markdown
Collaborator

@gcroci2 gcroci2 left a comment

Choose a reason for hiding this comment

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

I left minor comments, more for me to understrand than for suggesting changes. I removed the duplicated comments, but in case we decide to change something we need to remember that it may be in multiple files.

if isinstance (other, Atom):
return (self._residue == other._residue
and self._name == other._name)
return NotImplemented
Copy link
Copy Markdown
Collaborator

@gcroci2 gcroci2 Sep 13, 2023

Choose a reason for hiding this comment

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

Doesn't bool give errors with the NotImplemented case?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I found this here, when researching something else (https://stackoverflow.com/questions/63503512/python-type-hinting-own-class-in-method). Not sure where I can find the formal correct way to format __eq__, but the commentors on this post all seem to agree that this is the correct way to do it.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

You could run mypy to verify that the type hints are right

@DaniBodor
Copy link
Copy Markdown
Collaborator Author

I left minor comments, more for me to understrand than for suggesting changes. I removed the duplicated comments, but in case we decide to change something we need to remember that it may be in multiple files.

Sorry, I was actually meaning to leave references to some of these changes when I made the PR, but then forgot :)

I added them now in reply to your comments

@DaniBodor DaniBodor requested a review from gcroci2 September 13, 2023 08:12
@DaniBodor
Copy link
Copy Markdown
Collaborator Author

DaniBodor commented Sep 13, 2023

3 additional changes:

  1. I used from __future__ import annotations to avoid the quotation marks around some of the type hints
  2. I improved docstrings and their formatting according to this convention: https://realpython.com/documenting-python-code/#class-docstrings.
  3. I moved SingleResidueVariant into residue.py as they are closely related classes.
    • I even wonder if it would make sense to make it a child class of Residue instead of taking the Residue as an attribute (but that would require more coding than I am currently willing to do for this).

You should be able to go through the commits one by one to see each change in isolation, which might make reviewing easier.

Note that because I have "automatic import sorting upon save" turned on, some additional imports are switched around as well.

@gcroci2
Copy link
Copy Markdown
Collaborator

gcroci2 commented Sep 13, 2023

3 additional changes:

  1. I used from __future__ import annotations to avoid the quotation marks around some of the type hints

  2. I improved docstrings and their formatting according to this convention: https://realpython.com/documenting-python-code/#class-docstrings.

  3. I moved SingleResidueVariant into residue.py as they are closely related classes.

    • I even wonder if it would make sense to make it a child class of Residue instead of taking the Residue as an attribute (but that would require more coding than I am currently willing to do for this).

You should be able to go through the commits one by one to see each change in isolation, which might make reviewing easier.

Note that because I have "automatic import sorting upon save" turned on, some additional imports are switched around as well.

Regarding 3., JFYTK, we're using the Google Style convention for docstrings. Nice that you moved SingleResidueVariant into residue.py, makes more sense. Regarding the possibility of making it a child of Residue, that also makes sense, not sure how much it's worth to spend time on doing it

@DaniBodor
Copy link
Copy Markdown
Collaborator Author

3 additional changes:

  1. I used from __future__ import annotations to avoid the quotation marks around some of the type hints

  2. I improved docstrings and their formatting according to this convention: https://realpython.com/documenting-python-code/#class-docstrings.

  3. I moved SingleResidueVariant into residue.py as they are closely related classes.

    • I even wonder if it would make sense to make it a child class of Residue instead of taking the Residue as an attribute (but that would require more coding than I am currently willing to do for this).

You should be able to go through the commits one by one to see each change in isolation, which might make reviewing easier.
Note that because I have "automatic import sorting upon save" turned on, some additional imports are switched around as well.

Regarding 3., JFYTK, we're using the Google Style convention for docstrings. Nice that you moved SingleResidueVariant into residue.py, makes more sense. Regarding the possibility of making it a child of Residue, that also makes sense, not sure how much it's worth to spend time on doing it

Thanks for the ref to google convention. It does still follow that as well. I wasn't sure whether the class itself should get a docstring or only the init method. But in both style guides, they each get a docstring.

@DaniBodor DaniBodor merged commit e03565f into main Sep 13, 2023
@DaniBodor DaniBodor deleted the molstruct_type_hints branch September 13, 2023 10:01
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.

2 participants