-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Add missing type hints - Part 1 #4775
Add missing type hints - Part 1 #4775
Conversation
042bae9
to
b3b006a
Compare
85786d2
to
42ba09b
Compare
…nsole.commands.env
0b23a9a
to
1ed885a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
One question, besides the whitespace nit-picking:
Can/should we turn on Mypy for the affected modules, now that they are fully hinted? As best I can tell, nothing is actually checking that these new hints are correct at the moment.
@@ -64,6 +64,7 @@ pytest-sugar = "^0.9" | |||
httpretty = "^1.0" | |||
zipp = { version = "^3.4", python = "<3.8"} | |||
deepdiff = "^5.0" | |||
typing-extensions = {version = "^4.0.0", python = "<3.8"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit, but we are doing spaces around the braces.
(Side note: why doesn't poetry honor with with poetry add
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this is how poetry add
inserted it, I would prefer leaving it like it is.
I let mypy inspect these modules. Some of the errors seems to be due to incorrect typings in the application code. This is why I suggest we first fix mypy errors there and have a look at the tests afterwards. It looks like there are also some tricky errors, e.g.
This is because the TL;DR: Let us postpone mypy checks for the tests modules. |
* add missing type hints to application code * add typehints to tests.config, tests.console.commands.debug, tests.console.commands.env * add typehints to tests.console.commands.plugin * add typehints to tests.self * add typehints to tests.console.commands.source * add typehints to tests.console.commands.about * add typehints tests.console.commands.add * add typehints to tests.console.commands.test_cache * add typehints to tests.console.commands.test_check * add typehints to tests.console.commands.test_config * add typehints to tests.console.commands.test_export * add typehints to tests.console.commands.test_init * add typehints to tests.console.commands.test_install * add typehints to tests.console.commands.test_lock * add typehints to tests.console.commands.test_new * add typehints to tests.console.commands.test_publish * add typehints to tests.console.commands.test_remove * add typehints to tests.console.commands.test_run * add typehints to tests.console.commands.test_search * add typehints to tests.console.commands.test_show * add typehints to tests.console.commands.test_version * add typehints to tests.console.test_application * add typehints to tests.console.conftest * add typehints to tests.console.conftest * add python version constraint for typing-extension * fix type hint for http fixture
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This is the first part of incremental PR with the goal to force type hints in application code and tests.
This PR adds missing typehints to:
The
conftest.py
in the tests root folder will be updated whenever needed.