Adding stubs for pytest-snapshot #13448
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
srittau
left a comment
srittau
left a comment
There was a problem hiding this comment.
Thanks!
Not a full review, but some remarks below.
stubs/pytest-snapshot/METADATA.toml
Outdated
| @@ -0,0 +1,8 @@ | |||
| version = "0.9.*" | |||
| upstream_repository = "https://github.com/joseph-roitman/pytest-snapshot" | |||
| requires = [] | |||
There was a problem hiding this comment.
This needs to require pytest. This will currently fail the stub uploader tests, but I will send a PR to add (done now)pytest to its whitelist.
There was a problem hiding this comment.
Addressed! Tests pass!
stubs/pytest-snapshot/METADATA.toml
Outdated
| version = "0.9.*" | ||
| upstream_repository = "https://github.com/joseph-roitman/pytest-snapshot" | ||
| requires = [] | ||
| partial_stub = false |
There was a problem hiding this comment.
Nit: This is the default and redundant.
stubs/pytest-snapshot/METADATA.toml
Outdated
|
|
||
| [tool.stubtest] | ||
| stubtest_requirements = [] | ||
| ignore_missing_stub = true |
There was a problem hiding this comment.
Please add an __init__.pyi file. Then the whole tool.stubtest section should become redundant.
There was a problem hiding this comment.
Addressed, but possibly incompletely. Is an empty __init__.pyi sufficient?
Upstream doesn't seem to expose anything from that file, so presumably yes.
There was a problem hiding this comment.
Is an empty
__init__.pyisufficient?
Yes.
Needed for python/typeshed#13448. I think pytest can definitely be considered trusted.
blast-hardcheese
force-pushed
the
pytest-snapshot
branch
from
9c7cd5e to
b609507
Compare
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
srittau
left a comment
srittau
left a comment
There was a problem hiding this comment.
Thanks, a few remarks below.
| import _pytest.config.argparsing | ||
| import pytest | ||
|
|
||
| PARAMETRIZED_TEST_REGEX: re.Pattern[str] |
There was a problem hiding this comment.
Nit:
| PARAMETRIZED_TEST_REGEX: re.Pattern[str] | |
| PARAMETRIZED_TEST_REGEX: Final[re.Pattern[str]] |
Final needs to be imported from typing.
|
|
||
| def pytest_addoption(parser: _pytest.config.argparsing.Parser) -> None: ... | ||
| @pytest.fixture | ||
| def snapshot(request: pytest.FixtureRequest) -> Generator[Snapshot, Any, None]: ... |
There was a problem hiding this comment.
We usually use Generator[Snapshot] (although many annotations haven't been updated since type var defaults have been introduced), which is equivalent to Generator[Snapshot, None, None]. If it's important to you that anything can be sent into the generator, use object instead of Any, which we used for ignored parameters.
|
|
||
| class Snapshot: | ||
| def __init__(self, snapshot_update: bool, allow_snapshot_deletion: bool, snapshot_dir: Path): ... | ||
| def __enter__(self): ... |
There was a problem hiding this comment.
| def __enter__(self): ... | |
| def __enter__(self) -> Self: ... |
(Self needs to be imported from typing_extensions.)
| @snapshot_dir.setter | ||
| def snapshot_dir(self, value: str | Path) -> None: ... | ||
| def assert_match(self, value: str | bytes, snapshot_name: str | Path) -> None: ... | ||
| def assert_match_dir(self, dir_dict: dict[Any, Any], snapshot_dir_name: str | Path) -> None: ... |
There was a problem hiding this comment.
We now require each use of Any to be documented with the accepted types/usage. If you are unsure what the correct types are, you can use Incomplete (from _typeshed) – which is an alias of Any – instead.
2 participants
Adding relevant types for pytest-snapshot
Long time user, first time contributor. Thanks for everything thus far!
There's an existing effort to add types here: joseph-roitman/pytest-snapshot#59
... but considering that it's been stale for over a year this may be a good option for the time being.