-
-
Notifications
You must be signed in to change notification settings - Fork 120
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
cleanup patches after test in pytest plugin (#1148) #1164
Conversation
Apparently there is an issue in the 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 Report
@@ 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.
|
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.
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] |
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.
Oh, this is now a type alias. Let's leave a TODO
item to use explicit TypeAlias
once [email protected]
is out.
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.
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?
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.
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?
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.
Yes, 0.910
does not support it yet, 0.920
will (hopefully).
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.
Are there other features coming in 0.920 that returns finds useful?
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.
I am not sure about ParamSpec
(it might not be released just yet), but it would remove decorator
plugin! 🎉
returns/contrib/pytest/plugin.py
Outdated
|
||
|
||
@contextmanager | ||
def _spy_error_handling() -> Generator[_ErrorsHandled, None, 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.
Looks like it is just Iterator[_ErrorsHandled]
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.
Indeed -- I assumed contextmanager
would complain -- let's test that hypothesis 🧑🔬 ...
returns/contrib/pytest/plugin.py
Outdated
return wraps(original)(wrapper) # type: ignore | ||
|
||
|
||
_ERROR_HANDLING_PATCHERS: Final = { # noqa: WPS407 |
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.
Do we need this to be mutable? Or will types.MappingProxyType
work here?
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.
Nope. Chose the dict
for simplicity. Can use a readonly proxy just fine
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.
Let's do this! We are kinda strict on mutability in this specific project 👍
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.
Naturally 😅 . It's a shameMappingProxyType
isn't a full frozendict
-like object, but we'll take it.
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.
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. |
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.
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] |
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.
I am not sure about ParamSpec
(it might not be released just yet), but it would remove decorator
plugin! 🎉
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:
Checklist
CHANGELOG.md
Related issues
Closes #1148