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

Refactor/init type hints #3038

Merged
merged 22 commits into from
Dec 20, 2021
Merged

Refactor/init type hints #3038

merged 22 commits into from
Dec 20, 2021

Conversation

macintoshpie
Copy link
Contributor

@macintoshpie macintoshpie commented Dec 15, 2021

Any background context you want to provide?

See linked issues

What's this PR do?

  • Adds mypy to tox and CI process
  • Small refactors to SEED to pass mypy type checking -- some cases I just added some comments to ignore type checking, and in others I removed outdated code
  • Adds type hints to seed/models/derived_columns.py for examples

How should this be manually tested?

N/A, but definitely look through the edited derived_columns.py file

What are the relevant tickets?

#3034

Screenshots (if appropriate)

Example of an untyped func with hints (VSCode)
Screen Shot 2021-12-15 at 2 47 05 PM

Example using type hints for derived_columns stuff
Screen Shot 2021-12-15 at 3 29 56 PM

steps:
-
uses: actions/checkout@v2
-
uses: actions/setup-python@v2
with:
python-version: '3.7'
python-version: '3.9'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SEED is on 3.9!

ignore_missing_imports = True

[mypy-seed.*.migrations.*]
ignore_errors = True
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ignoring migration files for now b/c they raise type issues that aren't helpful

@@ -150,7 +152,7 @@ def evaluate(self, parameters=None):
parameters = {}

self._transformer.set_params(parameters)
return self._parser.parse(self._expression)
return self._parser.parse(self._expression) # type: ignore[return-value]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great example of types not working well sometimes. mypy is unable to infer that the Tree has a decorator which evaluates the tree (in this case into a float)

@@ -31,6 +31,44 @@ To run flake locally call:

tox -e flake8

Python Type Hints
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking for feedback on this. Specifically:

  • If you don't use VSCode, what do you use? we should check if there's a suitable extension for it
  • Is the usage of types clear and sufficient?

Copy link
Contributor

@perryr16 perryr16 Dec 16, 2021

Choose a reason for hiding this comment

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

  • I use VSCode
  • I think I'm following. Typing is not mandatory but preferred going forward. If I run into issues I can always use # type: ignore

Copy link
Contributor

Choose a reason for hiding this comment

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

I use Sublime and it looks like there's a plugin already that ties mypy in: https://github.com/fredcallaway/SublimeLinter-contrib-mypy

Copy link
Member

@nllong nllong left a comment

Choose a reason for hiding this comment

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

🔥

CI currently runs static type checking on the codebase using `mypy <http://mypy-lang.org/>`_. For
your own IDE, we recommend the following extensions:

- VSCode: `Pylance <https://marketplace.visualstudio.com/items?itemName=ms-python.vscode-pylance>`_ (uses Microsoft's Pyright type checking)
Copy link
Member

Choose a reason for hiding this comment

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

👍

import sys
if sys.version_info[:3] == (2, 7, 3):
import threading
threading._DummyThread._Thread__stop = lambda x: 42
Copy link
Member

Choose a reason for hiding this comment

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

yikes, this was in there for a long time!!!

@@ -11,10 +11,7 @@
import math
import tempfile

try:
Copy link
Member

Choose a reason for hiding this comment

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

cleanup cleanup everybody cleanup

@macintoshpie macintoshpie merged commit 60c5686 into develop Dec 20, 2021
@macintoshpie macintoshpie deleted the refactor/init-type-hints branch December 20, 2021 14:58
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.

5 participants