feat: add types for mypy #101
Conversation
|
May I ask you to resolve the conflicts? 🙏 Thanks! |
|
The integration tests are failing on my Windows machine so I am not sure if os is the issue. Let's wait for the tests here. |
setup.cfg
Outdated
|
|
||
| [options.entry_points] | ||
| pytest11 = | ||
| docker = pytest_docker | ||
|
|
||
| [tool:pytest] | ||
| addopts = --verbose --pylint-rcfile=setup.cfg | ||
| # --pylint --pycodestyle | ||
| addopts = --verbose --mypy --pycodestyle |
There was a problem hiding this comment.
I'd expect the --mypy flag no to be enabled by default, just as --pycodestyle or --pylint
There was a problem hiding this comment.
I initially removed pylint since the usage of it was commented out and it really was broken when uncommented and run on master branch. Same for pycodestyle. With pycodestyle it was a 2min easy fix so I kept it. With pylint the issue seemed harder to fix and since it could not be used I removed the dependency. But it got me thinking that it can't be that hard and that I can't be that lazy so I added it back and fixed the issues with it.
Let me disagree in the most possible way please, that it should not be enabled by default. I kind of strongly think the opposite. The pylint, pycodestyle and now mypy are included to enforce some coding standards. If you want to test the code without it, feel free to disable it, but for the code to be accepted, the automatic tests should always pass with these tools enabled (and the code rules therefore be enforced). That being said, I am open to argument and changing my stands here if you would have a strong case for not enabling it by default.
One more thing to add, mypy is being added primarily because I got fond of adding # type: ignore in our code every time we import this library so even if we could technically disable pycodestyle and pylint in tests, I think that we kindof need mypy to be "happy" all the time, so we can rely on the type hints of the imported code used in our apps.
There was a problem hiding this comment.
The pylint is enabled again, so we can resolve this discussion, right?
There was a problem hiding this comment.
I'd just expect those not to be enabled by default from config files.
Totally agree that all those checks should be enforced for code to be accepted. IMHO pytest --mypy --pycodestyle --pylint should be the test instruction in CI, but not the default for local runs.
Just a comment, no strong opinion about it!
CHANGELOG.md
Outdated
| @@ -1,5 +1,11 @@ | |||
| # Changelog | |||
There was a problem hiding this comment.
Please remove this file completely, the Releases page is used instead 🙏 Or, change the version to 3.1.0. Or don´t touch this file in this PR, and I will remove this file in another PR.
There was a problem hiding this comment.
Okay, please remove the file in another PR I will undo my changes.
This is already supported in the code, just fixing a regression in the type annotations from avast#101
This is already supported in the code, just fixing a regression in the type annotations from #101
No description provided.