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

cleanup patches after test in pytest plugin (#1148) #1164

Merged
merged 3 commits into from
Dec 7, 2021
Merged

cleanup patches after test in pytest plugin (#1148) #1164

merged 3 commits into from
Dec 7, 2021

Conversation

ariebovenberg
Copy link
Contributor

I have made things!

As discussed in #1148 and #1147

excuse the large diff; I took the liberty of rearranging the functions in the module to:

  • group them together by functionality
  • remove the need for a class which only had staticmethods

Checklist

  • I have double checked that there are no unrelated changes in this pull request (old patches, accidental config files, etc)
  • I have created at least one test case for the changes I have made
  • I have updated the documentation for the changes I have made
  • I have added my changes to the CHANGELOG.md

Related issues

Closes #1148

@ariebovenberg
Copy link
Contributor Author

ariebovenberg commented Dec 7, 2021

Apparently there is an issue in the m2r dependency chain. Reports are coming in, see miyakogi/m2r#66.

For now pinned the offending dependency. Probably best to create an issue (label: "good first issue") so we don't forget in the future

@codecov
Copy link

codecov bot commented Dec 7, 2021

Codecov Report

Merging #1164 (7d8bd0f) into master (cd28808) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1164   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           79        79           
  Lines         2415      2415           
  Branches       208       208           
=========================================
  Hits          2415      2415           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cbd9f1...7d8bd0f. Read the comment docs.

@ariebovenberg ariebovenberg marked this pull request as ready for review December 7, 2021 13:17
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Great! Thank you!

A couple of questions from me 🙂

@@ -28,7 +30,7 @@
# Also, the object itself cannot be (in) the key because
# (1) we cannot always assume hashability and
# (2) we need to track the object identity, not its value
_ERRORS_HANDLED: Final[Dict[int, Any]] = {} # noqa: WPS407
_ErrorsHandled = Dict[int, Any]
Copy link
Member

Choose a reason for hiding this comment

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

Oh, this is now a type alias. Let's leave a TODO item to use explicit TypeAlias once mypy@0.920 is out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was doubting as well whether to use it. Based on the PEP it seemed optional -- necessary in case of forward references, for example.

On the other hand, if this is looking to be the default way of declaring type aliases (explicit!) then we can already import from typing_extensions right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm something strange in mypy indeed, it gives:

Variable "typing_extensions.TypeAlias" is not valid as a type

mypy doesn't support them yet at all yet?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, 0.910 does not support it yet, 0.920 will (hopefully).

Copy link
Contributor Author

@ariebovenberg ariebovenberg Dec 7, 2021

Choose a reason for hiding this comment

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

Are there other features coming in 0.920 that returns finds useful?

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about ParamSpec (it might not be released just yet), but it would remove decorator plugin! 🎉



@contextmanager
def _spy_error_handling() -> Generator[_ErrorsHandled, None, None]:
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it is just Iterator[_ErrorsHandled]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed -- I assumed contextmanager would complain -- let's test that hypothesis 🧑‍🔬 ...

return wraps(original)(wrapper) # type: ignore


_ERROR_HANDLING_PATCHERS: Final = { # noqa: WPS407
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this to be mutable? Or will types.MappingProxyType work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. Chose the dict for simplicity. Can use a readonly proxy just fine

Copy link
Member

Choose a reason for hiding this comment

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

Let's do this! We are kinda strict on mutability in this specific project 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Naturally 😅 . It's a shameMappingProxyType isn't a full frozendict-like object, but we'll take it.

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Great work!

@@ -20,3 +20,7 @@ hypothesis==6.30.0
# TODO: Remove this lock when we found and fix the route case.
# See: https://github.com/typlog/sphinx-typlog-theme/issues/22
jinja2==3.0.3

# TODO: Remove this lock when this dependency issue is resolved.
Copy link
Member

Choose a reason for hiding this comment

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

Extra thanks for this! 👍

@@ -28,7 +30,7 @@
# Also, the object itself cannot be (in) the key because
# (1) we cannot always assume hashability and
# (2) we need to track the object identity, not its value
_ERRORS_HANDLED: Final[Dict[int, Any]] = {} # noqa: WPS407
_ErrorsHandled = Dict[int, Any]
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about ParamSpec (it might not be released just yet), but it would remove decorator plugin! 🎉

@sobolevn sobolevn merged commit c961ebf into dry-python:master Dec 7, 2021
@ariebovenberg ariebovenberg deleted the 1148-pytest-monkeypatch branch December 8, 2021 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Improved monkeypatching in pytest plugin
2 participants