-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
Adding stubs for pytest-snapshot #13448
base: main
Are you sure you want to change the base?
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: This is the default and redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
stubs/pytest-snapshot/METADATA.toml
Outdated
|
||
[tool.stubtest] | ||
stubtest_requirements = [] | ||
ignore_missing_stub = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an __init__.pyi
file. Then the whole tool.stubtest
section should become redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is an empty
__init__.pyi
sufficient?
Yes.
Needed for python/typeshed#13448. I think pytest can definitely be considered trusted.
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. 🤖🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, a few remarks below.
import _pytest.config.argparsing | ||
import pytest | ||
|
||
PARAMETRIZED_TEST_REGEX: re.Pattern[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.