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

Support static type checking #59

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

lonelyteapot
Copy link

@lonelyteapot lonelyteapot commented Sep 11, 2022

This pull request improves type annotations in the package, making it PEP 561-compatible.

In details:

  • Adds py.typed marker.
  • Adds mypy and pyright tox environments to ensure full compatibility with those tools.
  • Removes old-style type comments.
  • Adds type annotations to all publicly exposed methods.
  • Adds type annotations to internal utility methods.

Thanks to @mjpieters for contributing some of the code and ideas.

If you're unfamiliar with mypy, check these materials:
https://dev.to/whtsky/don-t-forget-py-typed-for-your-typed-python-package-2aa3
https://mypy.readthedocs.io/en/latest/installed_packages.html#creating-pep-561-compatible-packages

@mjpieters
Copy link

I came here to see if there was already an effort to provide typehints for this package, great to find a PR!

Have you considered adding a github workflow step and / or a tox environment to verify the hints? Otherwise it'll be hard to maintain the annotations long term.

Copy link

@mjpieters mjpieters left a comment

Choose a reason for hiding this comment

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

Can you also add a type hint for pytest_addoption()?

if TYPE_CHECKING:
    from _pytest.config.argparsing import Parser

def pytest_addoption(parser: Parser) -> None:

and for the snapshot() fixture:

def snapshot(request: pytest.FixtureRequest) -> Iterator["Snapshot"]:

(import typing.Iterator). This, incidentally, shows that there is a bug here; it makes the assumption FixtureRequest.node is always a Function node. It could do with an assertion or check to validate that assumption!

The __exit__ method also needs type hints; just replace the signature with:

    def __exit__(self, *_: Any) -> None:

as the exception info is ignored.

For _snapshot_path:

    def _snapshot_path(self, snapshot_name: Union[str, os.PathLike[str]]) -> Path:

If you were to apply all my review notes, then pyright gives this project a full 100% score on type completeness:

pyright --verifytypes pytest_snapshot  --ignoreexternal
Module name: "pytest_snapshot"
Package directory: ...
Module directory: ...
Path of py.typed file: ...

Public modules: 2
   pytest_snapshot
   pytest_snapshot.plugin

Symbols used in public interface:

Symbols exported by "pytest_snapshot": 10
  With known type: 10
  With ambiguous type: 0
  With unknown type: 0
    (Ignoring unknown types imported from other packages)
  Functions without docstring: 3
  Functions without default param: 0
  Classes without docstring: 1

Other symbols referenced but not exported by "pytest_snapshot": 0
  With known type: 0
  With ambiguous type: 0
  With unknown type: 0

Type completeness score: 100%

--ignoreexternal is needed because pytest types would otherwise affect the score.

There are some other things mypy, using the default config, will complain about; inside assert_match the snapshot_diff_msg needs a typehint to silence one of those:

             if encoded_expected_value is not None:
                 expected_value = decode(encoded_expected_value)
+                snapshot_diff_msg: Optional[str]
                 try:

That lets mypy know assigning None first doesn't mean you can't make it a str later on.

The remaining three I'd probably just silence with ignores. Or investigate if they could be a problem.

pytest_snapshot/_utils.py Outdated Show resolved Hide resolved
pytest_snapshot/_utils.py Outdated Show resolved Hide resolved
pytest_snapshot/_utils.py Outdated Show resolved Hide resolved
pytest_snapshot/plugin.py Outdated Show resolved Hide resolved
pytest_snapshot/plugin.py Outdated Show resolved Hide resolved
pytest_snapshot/plugin.py Outdated Show resolved Hide resolved
pytest_snapshot/plugin.py Outdated Show resolved Hide resolved
pytest_snapshot/_utils.py Outdated Show resolved Hide resolved
@lonelyteapot
Copy link
Author

Thanks for the review! I'll try to look into it within a week.

@lonelyteapot lonelyteapot changed the title Mark package as statically typed Support static type checking Dec 6, 2022
@lonelyteapot
Copy link
Author

Ready for review. Description updated. Here's some comments:

  1. There's 3 ignore comments, mainly because mypy can't fully figure out recursive dictionaries. It probably wasn't necessary to annotate utility functions anyway.
  2. I'm not that familiar with running tox on GitHub. It should work, but please double check the new environments.
  3. pytest 7.2.0 no longer depends on the py library, and mypy shows some errors when it's not installed.

I was hoping this would be a quick small improvement to the library.
@joseph-roitman, looking forward to the review. Could you please check this?

Copy link

codecov bot commented Nov 11, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (48079e2) 100.00% compared to head (f9da300) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##            master       #59   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          468       485   +17     
  Branches        53        53           
=========================================
+ Hits           468       485   +17     
Files Coverage Δ
pytest_snapshot/_utils.py 100.00% <100.00%> (ø)
pytest_snapshot/plugin.py 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@joseph-roitman joseph-roitman left a comment

Choose a reason for hiding this comment

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

Hello, thank you for the great PR!
Sorry for the late review. I fixed a few small issues and this can be merged. Could you squash everything into a single commit?
Once it is squashed, I'll merge the PR.

@bluetech
Copy link

bluetech commented Dec 5, 2023

It will be really great to have this (I was thinking to implement this myself before I saw @lonelyteapot already did the work).

@joseph-roitman Perhaps you can use GitHub's "Squash and merge" button, then you don't need to wait for @lonelyteapot to do it.

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.

4 participants