Skip to content
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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

blast-hardcheese
Copy link

@blast-hardcheese blast-hardcheese commented Jan 31, 2025

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.

This comment has been minimized.

blast-hardcheese added a commit to replit/river-python that referenced this pull request Jan 31, 2025

This comment has been minimized.

blast-hardcheese added a commit to replit/river-python that referenced this pull request Jan 31, 2025
blast-hardcheese added a commit to replit/river-python that referenced this pull request Feb 1, 2025

This comment has been minimized.

Copy link
Collaborator

@srittau srittau left a 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.

@@ -0,0 +1,8 @@
version = "0.9.*"
upstream_repository = "https://github.com/joseph-roitman/pytest-snapshot"
requires = []
Copy link
Collaborator

@srittau srittau Feb 27, 2025

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 pytest to its whitelist. (done now)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed! Tests pass!

version = "0.9.*"
upstream_repository = "https://github.com/joseph-roitman/pytest-snapshot"
requires = []
partial_stub = false
Copy link
Collaborator

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed


[tool.stubtest]
stubtest_requirements = []
ignore_missing_stub = true
Copy link
Collaborator

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.

Copy link
Author

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.

Copy link
Collaborator

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.

srittau added a commit to typeshed-internal/stub_uploader that referenced this pull request Feb 27, 2025
Needed for python/typeshed#13448. I think pytest can definitely be
considered trusted.

This comment has been minimized.

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Collaborator

@srittau srittau left a 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]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
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]: ...
Copy link
Collaborator

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): ...
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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: ...
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants