Use a match statement to dispatch on gherkin Child fields#822
Use a match statement to dispatch on gherkin Child fields#822Pierre-Sassoulas wants to merge 4 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #822 +/- ##
==========================================
- Coverage 96.12% 96.06% -0.06%
==========================================
Files 55 55
Lines 2423 2441 +18
Branches 136 136
==========================================
+ Hits 2329 2345 +16
- Misses 57 59 +2
Partials 37 37 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| document.feature.children.append(Child()) | ||
| return document | ||
|
|
||
| monkeypatch.setattr(parser_module, "get_gherkin_document", get_gherkin_document_with_unknown_child) |
There was a problem hiding this comment.
Do we really need to mock? Not clear to me what's going on and why we need this, and how it relates to the change made
There was a problem hiding this comment.
It's done in order to cover all the code branches to make codecov happy. With current gherkin code never produce a Child with none of background/rule/scenario set, so no real .feature file can exercise the fall-through branch. (I can drop it if you want)
|
If we adopt the assert_never suggestion, we shouldn't need the test, as mypy would cover it |
|
mypy doesn't understand it (yet?) sadly. |
| # An empty Child (a child kind newer than this gherkin version); | ||
| # skip it, like unknown children were skipped before the match. | ||
| pass | ||
| case unreachable: # pragma: no cover |
There was a problem hiding this comment.
Although I wonder, if they add new types, maybe we should gracefully ignore them, but add a warning message? But our CI wouldn’t break so we wouldn’t really see it… but i think maybe it’s for the best, so that we have less maintenance burden.
There was a problem hiding this comment.
Yeah, I think the warning message make sense. If it triggers it's going to be a lot of duplicate issue opened, but it's probably going to be duplicate issue in any case, just less obvious. Let me know if I should add it
There was a problem hiding this comment.
Yeah I'd say to remove the assert_never and instead have a catch-all case that emits a warning
A gherkin Child has exactly one of background/rule/scenario set; class patterns make that dispatch explicit and replace the truthiness checks with type matching.
A Child with none of background/rule/scenario set (a child kind a future gherkin version could introduce) must be skipped by FeatureParser.parse. No real feature file can produce one today, so the test appends a synthetic empty Child to the parsed document.
…k test assert_never on the Child dataclass itself does not typecheck: mypy cannot prove exhaustiveness from attribute patterns on a class with three optional fields, so the fall-through arm keeps the type "Child" instead of "Never". Matching on `child.background or child.rule or child.scenario` gives mypy a proper union (Background | Rule | Scenario | None) to exhaust; removing any arm is now a mypy error. Both defensive arms (empty Child, assert_never) are unreachable with the current gherkin version, so they are excluded from coverage instead of being exercised through a monkeypatched document, and the mock-based test is removed. "pragma: no cover" is re-added to coverage's exclude_lines: setting exclude_lines in the config replaces coverage.py's defaults, so the pragma had silently stopped working.
Pierre-Sassoulas
force-pushed
the
refactor/match-gherkin-child
branch
from
d01b886 to
0bc1bdf
Compare
Per review feedback, replace the assert_never fall-through (which would crash on a gherkin child kind newer than this version) with a catch-all case that emits a UserWarning and skips the child. pytest-bdd keeps working against future gherkin releases; the warning surfaces the gap without breaking CI. The arm is now reachable and exercised by a test, so the pragma: no cover markers are dropped.
Pierre-Sassoulas
force-pushed
the
refactor/match-gherkin-child
branch
from
91f5eb9 to
b837fc9
Compare
2 participants
Opinionated change following #821. A gherkin Child has exactly one of background/rule/scenario set; class patterns make that dispatch explicit and replace the truthiness checks with type matching.