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

Added type hints to most classes using mokeytype #306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HitLuca
Copy link

@HitLuca HitLuca commented Mar 3, 2022

Resolves #275

Pretty straightforward process, I installed monkeytype and ran monkeytype run -m unittest in order to run all tests and generate a database of call stacks.

With a small bash loop I applied type hints to all files:

for MODULE in $(monkeytype list-modules)
do
   monkeytype apply $MODULE
done

just to be sure, I installed mypy and checked both the flask_cors and the tests packages for typing issues (mypy flask_cors tests): out of a total of 35 errors, 12 were regarding the use of unittest.main() in each test file, the others seem to not be of concern and could be fixed by ensuring stricter type hints. Below is a list of all the errors found

flask_cors/core.py:113: error: Item "str" of "Union[Pattern[Any], str]" has no attribute "pattern"
flask_cors/core.py:120: error: Unsupported right operand type for in ("Optional[Any]")
flask_cors/core.py:133: error: Argument 2 to "try_match_any" has incompatible type "Optional[Any]"; expected "List[Union[str, Pattern[Any]]]"
flask_cors/core.py:158: error: Item "None" of "Optional[Any]" has no attribute "__iter__" (not iterable)
flask_cors/core.py:172: error: Argument 2 to "try_match_any" has incompatible type "Union[str, List[str], int, None]"; expected "List[Union[str, Pattern[Any]]]"
flask_cors/core.py:183: error: Need type annotation for "headers"
flask_cors/core.py:203: error: Unsupported right operand type for in ("Optional[Any]")
flask_cors/core.py:221: error: Argument 1 to "len" has incompatible type "Optional[Any]"; expected "Sized"
flask_cors/core.py:223: error: Argument 2 to "map" has incompatible type "Optional[Any]"; expected "Iterable[Any]"
flask_cors/core.py:250: error: Argument 2 to "get_cors_headers" has incompatible type "Headers"; expected "EnvironHeaders"
flask_cors/core.py:266: error: Unsupported right operand type for in ("Union[Pattern[Any], str]")
flask_cors/core.py:283: error: Incompatible return value type (got "Optional[typing.Match[str]]", expected "Union[difflib.Match, bool, None]")
flask_cors/core.py:285: error: Incompatible return value type (got "Union[typing.Match[Any], None, typing.Match[str]]", expected "Union[difflib.Match, bool, None]")
flask_cors/core.py:288: error: Item "Pattern[Any]" of "Union[Pattern[Any], str]" has no attribute "lower"
flask_cors/core.py:341: error: Item "None" of "Optional[str]" has no attribute "upper"
flask_cors/core.py:353: error: Incompatible return value type (got "Union[List[str], str, Set[str]]", expected "Union[List[str], List[Pattern[Any]], Set[str]]")
flask_cors/core.py:370: error: Argument 1 to "sanitize_regex_param" has incompatible type "Optional[Any]"; expected "Union[List[str], Set[str], Pattern[Any], str]"
flask_cors/core.py:371: error: Argument 1 to "sanitize_regex_param" has incompatible type "Optional[Any]"; expected "Union[List[str], Set[str], Pattern[Any], str]"
flask_cors/extension.py:19: error: Module "urllib" has no attribute "unquote_plus"
flask_cors/extension.py:19: error: Name "unquote_plus" already defined (possibly by an import)
flask_cors/extension.py:176: error: Cannot assign to a method
flask_cors/extension.py:178: error: Cannot assign to a method
tests/base_test.py:46: error: "FlaskCorsTestCase" has no attribute "app"
tests/extension/test_app_extension.py:382: error: Name "unittest" is not defined
tests/decorator/test_w3.py:71: error: Name "unittest" is not defined
tests/decorator/test_vary_header.py:87: error: Name "unittest" is not defined
tests/decorator/test_origins.py:204: error: Name "unittest" is not defined
tests/decorator/test_options.py:81: error: Name "unittest" is not defined
tests/decorator/test_methods.py:61: error: Name "unittest" is not defined
tests/decorator/test_max_age.py:60: error: Name "unittest" is not defined
tests/decorator/test_expose_headers.py:44: error: Name "unittest" is not defined
tests/decorator/test_duplicate_headers.py:33: error: Name "unittest" is not defined
tests/decorator/test_credentials.py:64: error: Name "unittest" is not defined
tests/decorator/test_allow_headers.py:107: error: Name "unittest" is not defined
tests/core/test_override_headers.py:33: error: Name "unittest" is not defined

There are a number of vague type hints (Any, Callable) which could be made more specific, but for now even having vague hints is better than nothing

@corydolphin
Copy link
Owner

Hey Luca! Thank you very much for the contribution! I am a big fan of MyPy but don't have experience using it in packaged libraries. Are you familiar with the best practices in terms of maintaining Python version compatibility? If so, I'd love some help with that side of things and then we can merge this and release!

@HitLuca
Copy link
Author

HitLuca commented Mar 14, 2022

The way I do it is to simply add a type checking step in the CI build step, below is an example of what I used to do with travis:

language: python
python:
  - "3.something"

install:
  - install_your_packages_etc

script:
  - your black, pylint, pytest, and other build step tests
  - mypy --strict my_project tests # make mypy strictly check type hints

If your CI pipeline runs against multiple python versions and mypy becomes unsupported in the future you just won't pass the build step and can react accordingly.


All the missing steps to make this project fully compliant with mypy:

  • add missing type hints, fixing the errors that pop up when running mypy flask_cors tests. We need to try and be as specific as possible, since adding my_var: Any doesn't improve the type hinting except removing a mypy typing error. If some types are hard to define, we can keep them as Any and refine them later down the road.
  • when the project is fully typed, add a py.typed file in order to be PEP 561 compliant and let mypy know that it can check the project types when it's used as an external package
  • add type checking in the build step, as seen above. Initially just a mypy flask_cors tests entry should suffice

If we want to be extra fancy, we can run mypy in strict mode with the --strict flag, both when checking types locally and in the CI pipeline: It will be very picky about everything, but once those errors are gone you are fully done:

Standard mypy likes this, as it's typed

def test(a: str) -> list:
    return a.split(",")

Strict mypy needs the type hints to be fully defined, so no list, dict, tuple, etc. Instead you need to specify what types they will accept

from typing import List


def test(a: str) -> List[str]:
    return a.split(",")

Let me know if something is unclear! Happy to contribute more, I just can't ensure consistency as I have a ton of other stuff to do

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.

Type hint project
2 participants