Make new-style wrappers use explicit wrapper=True#411
Merged
bluetech merged 1 commit intopytest-dev:mainfrom Jun 21, 2023
Merged
Make new-style wrappers use explicit wrapper=True#411bluetech merged 1 commit intopytest-dev:mainfrom
wrapper=True#411bluetech merged 1 commit intopytest-dev:mainfrom
Conversation
nicoddemus
approved these changes
Jun 20, 2023
Member
nicoddemus
left a comment
There was a problem hiding this comment.
Great work! Left a few comments.
docs/index.rst
Outdated
| be called to *wrap* (or surround) all other normal *hookimpl* calls. A *hook | ||
| wrapper* can thus execute some code ahead and after the execution of all | ||
| corresponding non-wrappper *hookimpls*. | ||
| A *hookimpl* can be marked with the ``"hookwrapper"`` option, which indicates |
Member
There was a problem hiding this comment.
Suggested change
| A *hookimpl* can be marked with the ``"hookwrapper"`` option, which indicates | |
| A *hookimpl* can be marked with the ``"wrapper"`` option, which indicates |
docs/index.rst
Outdated
| @@ -368,17 +368,17 @@ Wrappers | |||
| 1.1. For earlier versions, see the "old-style hook wrappers" section below. | |||
| The two styles are fully interoperable. | |||
Member
There was a problem hiding this comment.
We should mention the distinct option names next to each other, as this will help users search for them and be told of the difference explicitly.
Suggested change
| The two styles are fully interoperable. | |
| New-style hooks wrappers are declared with ``wrapper=True``, while old-style hook wrappers are declared with ``hookwrapper=True`` (for backward compatibility). | |
| The two styles are fully interoperable between plugins using different styles. However within the same plugin we recommend using only one style, for consistency. |
Member
Author
There was a problem hiding this comment.
Thanks! I'll apply this, except for the "(for backward compatibility)" part, I think it is potentially confusing.
Member
There was a problem hiding this comment.
Ahh right, might give the impression we are deprecating old-style hook wrappers, which is not the case.
| class Api: | ||
| @hookspec | ||
| def hello(self) -> Iterator[int]: | ||
| raise NotImplementedError() |
Member
There was a problem hiding this comment.
We could add raise NotImplementedError to .coveragerc as a line to be excluded from coverage.
testing/test_pluginmanager.py
Outdated
| class hello: | ||
| @hookimpl(wrapper=True, hookwrapper=True) | ||
| def he_method1(self): | ||
| yield |
Member
There was a problem hiding this comment.
Suggested change
| yield | |
| yield # pragma: no cover |
Previously new-style wrappers were implied by the function being a generator, however that broke a use case of a non-wrapper impl being a generator as a way to return an Iterator result. This is legitimate, and used at least by conda (see pytest-dev#403). Therefore, change to require an explicit `wrapper=True`. Also adds a test for the iterator-returning case. Fix pytest-dev#405.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously new-style wrappers were implied by the function being a generator, however that broke a use case of a non-wrapper impl being a generator as a way to return an Iterator result. This is legitimate, and used at least by conda (see #403).
Therefore, change to require an explicit
wrapper=True. Also adds a test for the iterator-returning case.Fix #405.