-
Notifications
You must be signed in to change notification settings - Fork 221
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
fix(typing): improve decorator type hinting #646
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #646 +/- ##
==========================================
+ Coverage 95.45% 95.58% +0.12%
==========================================
Files 49 49
Lines 1783 1788 +5
Branches 195 193 -2
==========================================
+ Hits 1702 1709 +7
+ Misses 53 52 -1
+ Partials 28 27 -1 ☔ View full report in Codecov by Sentry. |
src/pytest_bdd/steps.py
Outdated
@@ -74,10 +74,10 @@ def get_step_fixture_name(step: Step) -> str: | |||
|
|||
def given( | |||
name: str | StepParser, | |||
converters: dict[str, Callable] | None = None, | |||
converters: dict[str, Callable[[Any], Any]] | 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.
ditto
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'm not sure whether Callable[P, T]
is what is needed in this case. What would that imply for:
@given(
"Some step",
converts={
"id": int,
"valid": bool,
"list": lambda s: s.split(", "),
},
)
Having a single Callable[P, T]
would imply that they all have the same return type which is clearly incorrect.
Having said that, it might be correct to say that this should be refined to Callable[[str], Any]
, unless you are aware of cases where something else than a string is passed to the convert?
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.
ah right, indeed it should be Callable[[str], Any]
While I agree completely that Given you are using |
The type hinting for the most commonly used decorators were incomplete, resulting in decorated functions being obscured. This makes use of the special type variable `ParamSpec` which allows the type hinting a view into the parameters of a function. As ``ParamSpec` was introduced in Python 3.10, `ParamSpec` is imported from the `typing_extensions` module instead of the standard library. I have also taken the opportunity to fix other instances of `Callable` type hints missing their arguments. Signed-off-by: JP-Ellis <[email protected]>
The type hinting for the most commonly used decorators were incomplete, resulting in decorated functions being obscured.
This makes use of a
TypeVar
which allows the return type of the decorator to be the same as its input.I have also taken the opportunity to fix other instances of
Callable
type hints missing their arguments.PS:
The benefit of moving
import
statement behind theif TYPE_CHECKING
block is to avoid unnecessary parsing of libraries at runtime (when no type checking takes place). There were a few cases of:which provides no benefit as the
typing
library has to be parsed in the first place anyway.I hope it's alright that I fixed a couple of these :)