Skip to content

Conversation

@mathbunnyru
Copy link
Member

@mathbunnyru mathbunnyru commented Mar 7, 2022

What I've done:

  • Updated mypy config to use --strict --follow-imports error. This makes this tool much more efficient in finding errors.
  • Fixed all the errors
  • Made pre-commit mypy hook use the same config
  • Made sure the pre-commit hook works well and disabled mypy for git commit stage
  • Added some inline comments to explain why and how I did it.

@mathbunnyru
Copy link
Member Author

Unfortunately, pre-commit mypy doesn't work the same way as explicit mypy run.
So, it's better to run mypy --config-file ./mypy.ini ./ for now.
CC: @trallard

@consideRatio
Copy link
Member

Unfortunately, pre-commit mypy doesn't work the same way as explicit mypy run. So, it's better to run mypy --config-file ./mypy.ini ./ for now.

If we cant use if with pre-commit then that is also relevance to clarify in the file as a comment/fixme preferably linking out to something relevant to track if its to be fixed over time - a github issue in this or upstream repo.

@consideRatio
Copy link
Member

consideRatio commented Mar 7, 2022

Unfortunately, pre-commit mypy doesn't work the same way as explicit mypy run.
So, it's better to run mypy --config-file ./mypy.ini ./ for now.

If it doesn't work the same way, and it's not of importance if we choose way A or B but what is important is that we are consistent, I suggest we stick with anything that can be done with pre-commit instead of sticking with an option that requires manual intervention etc. I've not read up on mypy or what it does though or what difference there are etc.

@mathbunnyru mathbunnyru changed the title Update mypy config Update mypy config and make it work well with pre-commit Mar 8, 2022
@mathbunnyru
Copy link
Member Author

mathbunnyru commented Mar 8, 2022

@consideRatio thank you for very good suggestions.
I think I made it work well for pre-commit and documented the workaround.
Also I reflected all the changes in the PR message.

Please, take a look and tell me what you think.

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

Beautiful @mathbunnyru! Thanks for being so thorough with the transition to make this code typed, with all this setup I think this repo is really set to maintain typings!!! ❤️ 🎉

@mathbunnyru mathbunnyru merged commit 8f0a73e into jupyter:master Mar 8, 2022
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.

2 participants