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

Use TYPE_CHECKING instead of False #6435

Merged
merged 1 commit into from
Jan 16, 2020
Merged

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Jan 10, 2020

This allows for e.g. Jedi to infer types.

It was only used to support Python 3.5.0/3.5.1, where this is available
in typing_extensions.

Ref: davidhalter/jedi#1472

TODO:

@blueyed blueyed force-pushed the type_checking branch 2 times, most recently from dc46ddb to fcb9ca5 Compare January 10, 2020 19:17
src/_pytest/config/__init__.py Outdated Show resolved Hide resolved
@The-Compiler
Copy link
Member

Makes me wonder how common 3.5.0/3.5.1 really are in the wild - do we need/want to support them still? Also see some previous discussion in #5751.

@blueyed
Copy link
Contributor Author

blueyed commented Jan 10, 2020

Makes me wonder how common 3.5.0/3.5.1 really are in the wild - do we need/want to support them still? Also see some previous discussion in #5751.

I'd be ok with dropping support for it, but did not want to hold that back (also since we had issues/reports with < 3.5.2 before).
As per #5751 (comment) 3.5.2 appears to be used in Ubuntu LTS (which is OK).

@nicoddemus
Copy link
Member

Unless keeping the support becomes too terrible, I would rather we wait until we drop 3.5 completely.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks!

Please only add the CHANGELOG entry as requested, after that feel free to merge once CI passes. 👍

setup.py Outdated Show resolved Hide resolved
src/_pytest/compat.py Outdated Show resolved Hide resolved
@blueyed

This comment has been minimized.

blueyed added a commit to blueyed/pytest that referenced this pull request Jan 10, 2020
Python 3.5.0 caused flaky failures before
(pytest-dev#5795), hopefully this is
more stable.

This is pulled out of pytest-dev#6435,
which adds code specific for Python < 3.5.2, and therefore it makes
sense to test it.
@blueyed
Copy link
Contributor Author

blueyed commented Jan 10, 2020

I'll rebase this (with changelog) after merging master after #6440.

@The-Compiler

This comment has been minimized.

blueyed added a commit to blueyed/pytest that referenced this pull request Jan 11, 2020
Python 3.5.0 caused flaky failures before
(pytest-dev#5795), hopefully this is
more stable.

This is pulled out of pytest-dev#6435,
which adds code specific for Python < 3.5.2, and therefore it makes
sense to test it.
@blueyed blueyed changed the title Use TYPE_CHECKING instead of False [WIP/blocked] Use TYPE_CHECKING instead of False Jan 12, 2020
@bluetech
Copy link
Member

LGTM, as long as the dependency is only for Python<=3.5.1 it's fine by me :)

According to the PR description, this fixes jedi type inference. According to the linked issue, the jedi maintainer says they're going to stop supporting Python 3.5 in a few months. So things are going to break for these users anyway. But I guess they can always use an older jedi with newer pytest, so it's not a good reason to not fix the issue.

blueyed added a commit to blueyed/pytest that referenced this pull request Jan 12, 2020
Python 3.5.0 caused flaky failures before
(pytest-dev#5795).

This is pulled out of pytest-dev#6435,
which adds code specific for Python < 3.5.2.

It only runs a specific test, while collecting everything to get
coverage of the version specific code around typing.
blueyed added a commit to blueyed/pytest that referenced this pull request Jan 12, 2020
Python 3.5.0 caused flaky failures before
(pytest-dev#5795).

This is pulled out of pytest-dev#6435,
which adds code specific for Python < 3.5.2.

It only runs a specific test, while collecting everything to get
coverage of the version specific code around typing.
blueyed added a commit to blueyed/pytest that referenced this pull request Jan 14, 2020
Python 3.5.0 caused flaky failures before
(pytest-dev#5795).

This is pulled out of pytest-dev#6435,
which adds code specific for Python < 3.5.2.

It only runs a specific test, while collecting everything to get
coverage of the version specific code around typing.
@blueyed blueyed changed the title [WIP/blocked] Use TYPE_CHECKING instead of False [WIP] Use TYPE_CHECKING instead of False Jan 15, 2020
@blueyed
Copy link
Contributor Author

blueyed commented Jan 15, 2020

@bluetech

According to the PR description, this fixes jedi type inference. According to the linked issue, the jedi maintainer says they're going to stop supporting Python 3.5 in a few months. So things are going to break for these users anyway. But I guess they can always use an older jedi with newer pytest, so it's not a good reason to not fix the issue.

The point of this is that Jedi will actually infer types (it skips if False, but not if TYPE_CHECKING). So it fixes it also for 3.5.2+ - hope that clears things up.

@RonnyPfannschmidt
Copy link
Member

tbh, in addition i like the named constant better than the magic constant, lovely, thanks

@bluetech
Copy link
Member

Searching the jedi repo for typing_extensions brings up nothing, so I assume that the module isn't important, only the name. If so, can we avoid the runtime dep and just use TYPE_CHECKING = False for the fallback? I think mypy will be happy with this.

@blueyed
Copy link
Contributor Author

blueyed commented Jan 15, 2020

Ok, falls back to TYPE_CHECKING = False now.
(Jedi checks the name)

@blueyed
Copy link
Contributor Author

blueyed commented Jan 15, 2020

@RonnyPfannschmidt

tbh, in addition i like the named constant better than the magic constant, lovely, thanks

Do you mean we should use TYPE_CHECKING = False in compat always?

@RonnyPfannschmidt
Copy link
Member

@blueyed unless there is something making that a no go, i'd strongly prefer it over replicating it per file

@blueyed
Copy link
Contributor Author

blueyed commented Jan 15, 2020

@RonnyPfannschmidt
We have from typing import TYPE_CHECKING there now (for >= 3.5.2), not TYPE_CHECKING = False.

Anyway, this is still affected by circular imports now, fixed by avoiding it there, instead of moving imports to methods:

diff --git a/src/_pytest/outcomes.py b/src/_pytest/outcomes.py
index b08e7d2c9..0cd1072ef 100644
--- a/src/_pytest/outcomes.py
+++ b/src/_pytest/outcomes.py
@@ -8,7 +8,7 @@

 from packaging.version import Version

-from _pytest.compat import TYPE_CHECKING
+TYPE_CHECKING = False  # avoid circular import through compat

 if TYPE_CHECKING:
     from typing import NoReturn

src/_pytest/compat.py Outdated Show resolved Hide resolved
@blueyed blueyed changed the title [WIP] Use TYPE_CHECKING instead of False Use TYPE_CHECKING instead of False Jan 16, 2020
@blueyed
Copy link
Contributor Author

blueyed commented Jan 16, 2020

Should be good now.
Could be simplified when we decide to not support < 3.5.2 though, but is also trivial to do later of course.

@pytest-dev pytest-dev deleted a comment from codecov bot Jan 16, 2020
@pytest-dev pytest-dev deleted a comment from codecov bot Jan 16, 2020
@blueyed blueyed changed the base branch from features to master January 16, 2020 12:43
@blueyed
Copy link
Contributor Author

blueyed commented Jan 16, 2020

Rebased on master.
Needs/should be done for src/_pytest/config/argparsing.py then when merging into features.

This allows for e.g. Jedi to infer types (it checks the name).

It was only used to support Python 3.5.0/3.5.1, where this is is not
available in the `typing` module.

Ref: davidhalter/jedi#1472

Uses `TYPE_CHECKING = False` in `_pytest.outcomes` to avoid having to
work around circular import.
@blueyed blueyed merged commit 749752d into pytest-dev:master Jan 16, 2020
@blueyed blueyed deleted the type_checking branch January 16, 2020 18:45
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.

6 participants