Fix pytest_ignore_collect hooks: do not return False

[blueyed]

It should only return True when something is to be ignored, not
False otherwise typically.

This caused e.g. bad interaction with the cacheprovider (before
#6448).

[nicoddemus]

Can you elaborate a bit? I ask because I don't see anywhere in the code where we make a distinction between None and False when calling pytest_ignore_collect; they are all in the form:

if ihook.pytest_ignore_collect(path=path, config=self.config):
    ...

[blueyed]

It's a firstresult hook (and None is not a result there then).

[blueyed]

@nicoddemus please suggest how to improve the doc then even more - since it appears to be still unclear.

[nicoddemus]

Ahh got it, the key point is the fact the hook is firstresult, sorry my bad. 👍

[blueyed]

Ahh got it, the key point is the fact the hook is firstresult, sorry my bad.

Can this be improved in the doc then to make it clearer?

[nicoddemus]

Can this be improved in the doc then to make it clearer?

Perhaps this?

    As any other :ref:`firstresult` hook, this stops at first non-`None` result. This hook
    can return `False` explicitly to prevent a file to be ignored by other hooks.

[blueyed]

I've pulled out the doc change.
There are more places using the same sentence, and this should be fixed across all of them, maybe adding and linking to an intermediate doc section then.
=> #6787

[blueyed]

@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?)

[nicoddemus]

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]]":
    ...

[blueyed]

@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))

[bluetech]

@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.

[blueyed]

@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.

[nicoddemus]

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. 👍