Skip to content

Fix pytest_ignore_collect hooks: do not return False #6778

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Feb 22, 2020
Merged

Fix pytest_ignore_collect hooks: do not return False #6778

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cc03174
Fix pytest_ignore_collect hooks: do not return False
blueyed Feb 20, 2020
949ee2f
keep None
blueyed Feb 21, 2020
1aa8747
typing
blueyed Feb 21, 2020
00eb39d
revert doc
blueyed Feb 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/_pytest/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

if TYPE_CHECKING:
from typing import Type
from typing_extensions import Literal

from _pytest.python import Package

Expand Down Expand Up @@ -295,7 +296,9 @@ def _in_venv(path):
return any([fname.basename in activates for fname in bindir.listdir()])


def pytest_ignore_collect(path, config):
def pytest_ignore_collect(
path: py.path.local, config: Config
) -> "Optional[Literal[True]]":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bluetech I assume using Literal[True] here makes sense now? (although not in the hookspec itself)
(I've quickly checked your other typing PRs, but it was not included there, is it?)

Copy link
Member

Choose a reason for hiding this comment

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

General question: is the hook implementation allowed to declare a return type that is different, but still a subset, of the return type declared by the hookspec?

Example:

@hookspec
def pytest_ignore_collect(
    path: py.path.local, config: Config
) -> "Optional[bool]":
    ...

@hookimpl
def pytest_ignore_collect(
    path: py.path.local, config: Config
) -> "Optional[Literal[True]]":
    ...

Copy link
Contributor Author

@blueyed blueyed Feb 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@nicoddemus
Yes, this is allowed in general:

from typing import Literal
from typing import Optional


class A:
    def f(self) -> Optional[bool]:
        pass


class B(A):
    def f(self) -> Literal[True]:
        return True

reveal_type(A().f())  # Revealed type is 'Union[builtins.bool, None]'
reveal_type(B().f())  # Revealed type is 'Literal[True]'

(keep in mind though that mypy currently does not know that hookimpls are subsets of hookspecs (as with subclasses))

Copy link
Member

Choose a reason for hiding this comment

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

@blueyed

@bluetech I assume using Literal[True] here makes sense now? (although not in the hookspec itself)

There are arguments for and against.

For: the type provides a quick indication that this impl either only returns None or True. Can gain useful semantic information just from the type, namely that it doesn't return False.

Against: If sometime the circumstances change, and the function wants to start returning False, that's perfectly legit logically, but the type needs to change. This suggests the type is over restrictive.

Against: Might confuse the person who wants to make the change, if they don't realize they are allowed to extend the type. Might confuse copy/pasters. (Note: the argument is weakened by the fact hookimpls are already allowed to omit arguments they don't use, so might not match the hookspec already).

My inclination is to use the more general type and match the hookspec, but I can see both ways, and am probably not too consistent myself. So I'm fine with whatever you decide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bluetech
Thanks for elaborating on it again. I think the benefit of gaining useful semantic information just from the type is useful here in general.

Will wait for any other feedback, and otherwise merge it as is.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is allowed in general

Yes thanks, that's exactly what I meant to ask (not about hooks/impls, but in general).

Will wait for any other feedback, and otherwise merge it as is.

I'm fine with what you guys decide, and the current form. 👍

ignore_paths = config._getconftest_pathlist("collect_ignore", path=path.dirpath())
ignore_paths = ignore_paths or []
excludeopt = config.getoption("ignore")
Expand All @@ -319,8 +322,7 @@ def pytest_ignore_collect(path, config):
allow_in_venv = config.getoption("collect_in_virtualenv")
if not allow_in_venv and _in_venv(path):
return True

return False
return None


def pytest_collection_modifyitems(items, config):
Expand Down