Skip to content
This repository has been archived by the owner on Feb 21, 2023. It is now read-only.

Add mypy to CI #1099

Closed
wants to merge 5 commits into from
Closed

Add mypy to CI #1099

wants to merge 5 commits into from

Conversation

Andrew-Chen-Wang
Copy link
Collaborator

@Andrew-Chen-Wang Andrew-Chen-Wang commented Aug 7, 2021

What do these changes do?

Added mypy to linter of CI

One thing to note is if we want to allow failure. Not everyone wants mypy, and PRs could be hindered by them. Running them is great anyway, but maybe not necessary to block anything crucial.

Are there changes in behavior for the user?

No

Related issue number

Fixes #1098 and other comments made in the past that I'm not gonna go look for.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES/ folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example:
      Fix issue with non-ascii contents in doctest text files.

@Andrew-Chen-Wang Andrew-Chen-Wang mentioned this pull request Aug 7, 2021
- name: Run mypy
run: |
pip install -r tests/requirements-mypy.txt
mypy aioredis --ignore-missing-imports
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to just run mypy with no arguments, and keep all configuration in .mypy.ini.

Good example of a config from one of our other repos:
https://github.com/aio-libs/aiohttp-jinja2/blob/master/.mypy.ini

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah just copied our makefile; good catch since we have all dependencies installed. Not sure of the reasoning behind the flag to begin with though.

Copy link
Contributor

Choose a reason for hiding this comment

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

But, even the aioredis can be removed and just included in the config file as files = aioredis.

We'll also want mypy to run on tests/examples at some point as well. We've had bugs with type annotations which are only caught if you type check actual usage (plus it keeps the examples up-to-date with the library code).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't have a config file, so updated in CI only. Maybe in the future.

@Andrew-Chen-Wang
Copy link
Collaborator Author

looks like dream sorcerer got it working (prob because member of org). Closing since CI works in #1101.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable Mypy
2 participants