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

Improving Type Hinting #531

Merged
merged 4 commits into from
Jan 20, 2022
Merged

Conversation

john-science
Copy link
Member

@john-science john-science commented Jan 11, 2022

Description

This PR adds type hinting to an arbitrary subset of our code base; basically low-hanging fruit. This action was undertaken as part of this discussion: #493

The process taken to generate these type hints was relatively easy.

  1. Install monkeytype.
  2. Execute monkeytype run armi/path/to/test_whatever.py to determine type hints for functions.
    • This can be done on multiple test files, and all the type hints will be collated.
  3. Execute monkeytype stub armi.path.to.whatever to print-to-screen type hints.

This process is easy to do. However, it is not perfect:

  • Most of our unit test files cannot be run as-is using monkeytype, because they do no explicitly declare an App(), so without pytest they just fail.
  • Many of the type hints monkeytype output that are our custom classes are only pseudo-correct. For instance, in many places in our codebase a method might return any subclass of one of our custom classes, and monkeytype doesn't handle OOP that well. (Conversely, it might be that Python's type hinting is not well designed to handle this very common use case).
  • Some of the unit tests we run don't fully flex a given function, so 'monkeytype just doesn't have the info necessary to get the type hinting 100% right. I have found many partial errors.

It seems to me that monkeytype is a useful tool. So I am opening this PR just as an example of the benefits it can generate. But due to the issues I listed above, it was not an automatic enough process for me to just sweep the whole code base and commit everything.

@john-science john-science added documentation Improvements or additions to documentation enhancement New feature or request cleanup Code/comment cleanup: Low Priority labels Jan 11, 2022
Copy link
Member

@jakehader jakehader left a comment

Choose a reason for hiding this comment

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

Hi @john-science, this looks very reasonable and thanks for the description of the steps you took to get monkeytype to work. I just have one question.

@john-science john-science merged commit c6acb9d into terrapower:master Jan 20, 2022
@john-science john-science deleted the monkey_business branch January 20, 2022 03:39
scottyak pushed a commit to scottyak/armi that referenced this pull request Oct 27, 2022
@jakehader
Copy link
Member

Hi @john-science, this looks very reasonable and thanks for the description of the steps you took to get monkeytype to work. I just have one question.

How are you doing? Hilarious that I said I had a question and never asked it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code/comment cleanup: Low Priority documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants