-
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 #658
Fix typing #658
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #658 +/- ##
==========================================
- Coverage 96.08% 96.01% -0.08%
==========================================
Files 55 55
Lines 2250 2359 +109
Branches 246 250 +4
==========================================
+ Hits 2162 2265 +103
- Misses 53 56 +3
- Partials 35 38 +3 ☔ View full report in Codecov by Sentry. |
It was never supposed to return a callable with the same ParamSpec
Well, mypy is happy only because we put a lot of |
Good point - so the focus of this would be to eliminate those where possible :) |
# Conflicts: # poetry.lock # src/pytest_bdd/cucumber_json.py # src/pytest_bdd/feature.py # src/pytest_bdd/generation.py # src/pytest_bdd/gherkin_terminal_reporter.py # src/pytest_bdd/parser.py # src/pytest_bdd/reporting.py # src/pytest_bdd/scenario.py # src/pytest_bdd/steps.py # src/pytest_bdd/utils.py # tests/feature/test_description.py
6f116e7
to
f52341c
Compare
This way we are consistent with the other registries
# Conflicts: # CHANGES.rst # src/pytest_bdd/parser.py # src/pytest_bdd/scenario.py # src/pytest_bdd/utils.py
I managed to remove all occurrences of `Any`, and use proper typed dicts instead
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.
Copilot reviewed 5 out of 14 changed files in this pull request and generated no suggestions.
Files not reviewed (9)
- CHANGES.rst: Language not supported
- src/pytest_bdd/steps.py: Evaluated as low risk
- src/pytest_bdd/cucumber_json.py: Evaluated as low risk
- src/pytest_bdd/feature.py: Evaluated as low risk
- src/pytest_bdd/compat.py: Evaluated as low risk
- tests/feature/test_report.py: Evaluated as low risk
- src/pytest_bdd/generation.py: Evaluated as low risk
- src/pytest_bdd/parser.py: Evaluated as low risk
- src/pytest_bdd/plugin.py: Evaluated as low risk
Comments skipped due to low confidence (4)
src/pytest_bdd/utils.py:99
- The
default
parameter should be typed asV | T | None
to match the return type and avoid confusion.
def registry_get_safe(registry: WeakKeyDictionary[K, V], key: object, default: T | None = None) -> T | V | None:
src/pytest_bdd/scenario.py:289
- The type annotation for the decorator function is incorrect. It should be updated to match the new signature.
) -> Callable[[Callable[P, T]], Callable[P, T]]:
src/pytest_bdd/scenario.py:471
- [nitpick] The variable name 's' within the generator expression is not descriptive. It should be renamed to something more meaningful.
if (s := registry_get_safe(scenario_wrapper_template_registry, attr)) is not None
src/pytest_bdd/gherkin_terminal_reporter.py:77
- Ensure that
scenario
is notNone
before accessing its attributes to prevent potentialAttributeError
.
if self.verbosity <= 0 or scenario is None:
Fix many
mypy
issues.Notably, I replace private attributes like
__scenario__
,__scenario_report__
, etc. with registries (WeakKeyDictionary). E.g.Other changes:
Any
with a more specific type if possible, otherwiseobject
.Any
causes the type checker to completely forego the type checking. Usingobject
is a better way.