typing: pytest_collection #6601
Conversation
| @@ -158,7 +166,7 @@ def pytest_load_initial_conftests(early_config, parser, args): | |||
|
|
|||
|
|
|||
| @hookspec(firstresult=True) | |||
| def pytest_collection(session): | |||
| def pytest_collection(session: "Session") -> Optional[Any]: | |||
There was a problem hiding this comment.
Optional[Any]: the return values are not used, this is meant to indicate that.
blueyed
force-pushed
the
typing-pytest_collection
branch
from
c6e612f to
740a70c
Compare
src/_pytest/main.py
Outdated
| def _main( | ||
| config: Config, session: "Session" | ||
| ) -> Optional[ | ||
| Union["Literal"[ExitCode.TESTS_FAILED], "Literal"[ExitCode.NO_TESTS_COLLECTED]] |
There was a problem hiding this comment.
Are you sure _main will only ever return these two error codes? If it is put in the type annotation, then once they're published, it becomes an API guarantee.
There was a problem hiding this comment.
That's what is used in that function. Also _main is not really public API, is it?
There was a problem hiding this comment.
It's what used in the function but it's better to keep our options open IMO.
IIUC, eventually this return value is funneled to the main function in _pytest/config/__init__.py which has return type Union[int, _pytest.main.ExitCode]. My suggestion is to use the same type here.
There was a problem hiding this comment.
Sorry, I fail to see why. If a function returns only 1, or 2, it should/could have Literal[1, 2], even when used from another function that returns int, no?
There was a problem hiding this comment.
This is really not very important but I'll try to explain my view.
I think restricting the type in such a way is not beneficial. The function is an (indirect) implementation of a hook and the only restriction that needs to apply to it is that it implements the hook specification.
Unless this is an actual API guarantee that should not be changed without it being a breaking change, the fact that it currently returns these two values is just an incidental implementation detail. If someone wants to say add a new error code, then they 1) might be unsure if they're allowed to change the type, and 2) need to "uselessly" adjust the type.
There was a problem hiding this comment.
I've really seen it more like being able to see from the signature already, what it might "raise".
But I do not have a strong opinion on it, and certainly trust you more with regard to type hinting.. :)
Amended.
There was a problem hiding this comment.
@bluetech just for understanding: would you type a function always returning True also with bool then? (https://github.com/blueyed/pytest/blob/70481429f8236141e3a39098082b254f4a1cc017/src/_pytest/python.py#L172)
My general understanding/idea with that was that we could later tell that a certain hook, when used, would always skip later ones. This is somehow indicated with -> bool also (not Optional[bool], but following your argument it should be typed like the hookspec, which is Optional[…]/Optional[Any] then for firstresult=True.
There was a problem hiding this comment.
I'd tend to use the same type as the hookspec. I see that fact that the impl currently skips (returns True) is a runtime property, which might change. But if it's really fundamental, as in we want to "enforce" that this particular impl always skips, then making it -> True does make sense.
blueyed
force-pushed
the
typing-pytest_collection
branch
3 times, most recently
from
b4922c9 to
f587b28
Compare
blueyed
force-pushed
the
typing-pytest_collection
branch
from
f587b28 to
88b8003
Compare
Not included in #6556, but conflicts (trivially).