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

Enable mypy in precommit #318

Merged
merged 3 commits into from
Feb 3, 2022
Merged

Enable mypy in precommit #318

merged 3 commits into from
Feb 3, 2022

Conversation

hackaugusto
Copy link
Contributor

  • Adds mypy to the precommit
  • Fixes some typing issues with code that should have been type checked
  • Made the configuration for type checking opt-out instead of opt-in (the idea is that code in new modules should be typed by default)

@hackaugusto hackaugusto requested review from a team as code owners February 1, 2022 17:36
@hackaugusto
Copy link
Contributor Author

Notes:

  • Mypy treats repeated module names as an error. Because of theses, the conftest.py files used to configure pytest results in errors. The fix is to run mypy on each module independently Suppress 'duplicate module named xx' errors? python/mypy#4008, because of this I'm not yet enabling mypy for the test code
  • We support python >=3.7, user defined type guards were only introduced on python >=3.10, so for now the code has quite a few asserts for types

@hackaugusto hackaugusto marked this pull request as draft February 1, 2022 17:40
@jlprat
Copy link
Contributor

jlprat commented Feb 1, 2022

Do you think it would be worth to add this to CONTRIBUTING.md so people know are aware of this?

@hackaugusto
Copy link
Contributor Author

Do you think it would be worth to add this to CONTRIBUTING.md so people know are aware of this?

We do mention it, the problem was that mypy was not configured to run with pre-commit.

@hackaugusto
Copy link
Contributor Author

hackaugusto commented Feb 1, 2022

Note: to enable mypy for the tests, something like this should be used:

  # `mypy tests` can't be run because mypy complains about the repeated conftest.py
  # https://github.com/python/mypy/issues/4008: current solution is to run mypy separately
  - repo: https://github.com/pre-commit/mirrors-mypy
    rev: v0.931
    hooks:
      - id: mypy
        name: Mypy Karapace
        pass_filenames: false
        args: ["--ignore-missing-imports", "--package", "karapace"]

      - id: mypy
        name: Mypy integration tests
        pass_filenames: false
        args: ["--ignore-missing-imports", "tests/integration"]

      - id: mypy
        name: Mypy unit tests
        pass_filenames: false
        args: ["--ignore-missing-imports", "tests/unit"]

@hackaugusto
Copy link
Contributor Author

hackaugusto commented Feb 2, 2022

I did a quick look on how to model the JSON Schema using recursive types, but that is just not really feasible now for a few reasons:

  • To represent dictionaries with non-homegenous value types we need TypedDict which is only >=3.8 (otherwise we would need to add the typing extensions as a dependency and that has other issues)
    • Even if we had TypedDict, it seems that it is a closed model (unknown keys are not allowed), whereas the JSON Schema object is an open model (additional keys are ignored)
  • Recursion is not really supported by Mypy with type aliases, only with protocols, and as far as I can see those only works with classes, not dicts

@hackaugusto hackaugusto marked this pull request as ready for review February 2, 2022 11:24
Copy link
Contributor

@jongiddy jongiddy left a comment

Choose a reason for hiding this comment

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

Looks good, apart for one spelling error.

karapace/compatibility/__init__.py Outdated Show resolved Hide resolved
@jongiddy jongiddy merged commit a02047d into master Feb 3, 2022
@jongiddy jongiddy deleted the hacka-enable-mypy-in-precommit branch February 3, 2022 14:19
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.

3 participants