Add more type annotations #7142
Conversation
bluetech
force-pushed
the
typing
branch
4 times, most recently
from
03e01a7
to
872a779
Compare
bluetech
force-pushed
the
typing
branch
2 times, most recently
from
23c0e32
to
e2bb784
Compare
There was a problem hiding this comment.
Thanks @bluetech and congratulations on reaching a milestone on fully typing pytest. 😁
I'm approving with the caveat that I'm no typing expert, but the types at a glance seem correct, so I will defer to your judgment. 👍
Thanks again.
|
i started to read trough, and i'll continue tomorrow, so far i like what i see, great work, that must have been a total pain to get so far |
|
@nicoddemus @RonnyPfannschmidt Thanks a lot for reviewing this. I have a rebase ready, will push it after @RonnyPfannschmidt is finished reviewing so it doesn't interfere midway. |
|
Please push the rebase |
|
Since I didn't have a chance to look at that many files, I'd rather review the current state and catch rebase issues rather than continue on the old stuff |
|
@RonnyPfannschmidt sure, pushed! |
| @@ -465,27 +465,27 @@ def test_report_extra_parameters(reporttype: "Type[reports.BaseReport]") -> None | |||
|
|
|||
|
|
|||
| def test_callinfo() -> None: | |||
There was a problem hiding this comment.
followup note: the item below could probably be actually 3 tests
| @@ -877,7 +886,7 @@ def getini(self, name): | |||
| xmlpath = str(tmpdir.join("junix.xml")) | |||
| register = gotten.append | |||
|
|
|||
| fake_config = FakeConfig() | |||
| fake_config = cast(Config, FakeConfig()) | |||
There was a problem hiding this comment.
i wasn't aware that one works, its such a helpful tool to go that way with fake objects
| nodeid = "test_node_id" | ||
|
|
||
| log.pytest_sessionstart() | ||
| log.add_global_property("foo", 1) | ||
| log.add_global_property("bar", 2) | ||
| log.add_global_property("foo", "1") |
There was a problem hiding this comment.
i believe we need a followup to warn on non-string properties
| @dataclass | ||
| class SimpleDataObjectOne: | ||
| field_a: int = field() | ||
| field_b: int = field() | ||
| field_b: str = field() |
| assert w[0].lineno == inspect.currentframe().f_lineno - 1 | ||
| assert w[0].filename == __file__ | ||
| nodes.Node(name="test", config=ms, session=ms, nodeid="None") # type: ignore | ||
| assert w[0].lineno == inspect.currentframe().f_lineno - 1 # type: ignore |
There was a problem hiding this comment.
was the line drop here intentional?
There was a problem hiding this comment.
I don't quite remember but seems like it was not intentional, I'll add it back.
| @@ -367,13 +402,13 @@ def write(self, content: str, *, flush: bool = False, **markup: bool) -> None: | |||
| def flush(self) -> None: | |||
| self._tw.flush() | |||
|
|
|||
| def write_line(self, line, **markup): | |||
| def write_line(self, line: Union[str, bytes], **markup: bool) -> None: | |||
There was a problem hiding this comment.
do we have any way to ensure this will turn str only?
There was a problem hiding this comment.
pytest itself doesn't need the bytes support (linting & tests pass without it), but it's a public method so would be a breaking change technically. I can send a PR to drop it for 6.x if you think we should.
There was a problem hiding this comment.
We should note it for 7.x, 6.x is a bit close and won't give external users time to get the change, but a deprecation might be nice to add soon
| # Will return a dummy fixture if the setuponly option is provided. | ||
| if request.config.option.setupplan: | ||
| my_cache_key = fixturedef.cache_key(request) | ||
| fixturedef.cached_result = (None, my_cache_key, None) | ||
| return fixturedef.cached_result | ||
| return None |
There was a problem hiding this comment.
are those explicit returns annotations for mypy?
There was a problem hiding this comment.
Yes, mypy has a warning when if one code path has a return <value> then all code paths should.
|
Besides @RonnyPfannschmidt comment, since master updated to mypy 0.780 since last rebase, I also get a few new warnings which I'll fix: |
Annotate some "easy" arguments of hooks that repeat in a lot of internal plugins. Not all of the arguments are annotated fully for now.
The FixtureFunctionMarker attrs class already converts the params itself. When adding types, the previous converter composition causes some type error, but extracting it to a standalone function fixes the issue (a lambda is not supported by the mypy plugin, currently).
This option checks even functions which are not annotated. It's a good step to ensure that existing type annotation are correct. In a Pareto fashion, the last few holdouts are always the ugliest, beware.
Currently based on:
#6999, #7019. Not based on #7121 but would be nice to rebase on it once it's merged.#7253.This PR adds type annotations to many parts of pytest, enough that we can enable
check_untyped_defs = Truefor all of pytest (including tests). I worked on this intermittently since I started working on pytest, and I think it would be good to have for the planned 6.0 release.Types act as a "super linter", and makes the code easier to refactor and understand. The types themselves can sometimes be complex, but hopefully not too much, and I usually take it as a hint that the code should be simplified.
Most of the changes are mundane. I kept the code changes to a minimum -- mostly asserts and trivial variable renamings to make the annotations work.
I understand this is hard to review in detail. I hope however to get an ack, based mostly on trust. If this causes regressions or other issues I promise to handle that.
This PR doesn't yet "publish" the types (
py.typedfile). I'd like to do this too for 6.0, but I think it should be discussed separately, as it has implications for API stability (can type change, should types be exposed for purposes of annotation in user code, etc.) and rollout plans.