Skip to content
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

Remove Gherkin local dataclasses #759

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jsa34
Copy link
Collaborator

@jsa34 jsa34 commented Jan 1, 2025

Remove the custom definition of the gherkin document as dataclasses in pytest-bdd.

Bump the minimum supported version of gherkin-official to v30 to support this and remove the warnings in python 3.13+

…n pytest-bdd.

Bump the minimum supported version of gherkin-official to v30 to support this and remove the warnings in python 3.13+
Copy link

codecov bot commented Jan 1, 2025

Codecov Report

Attention: Patch coverage is 86.25000% with 11 lines in your changes missing coverage. Please review.

Project coverage is 95.34%. Comparing base (9ff6980) to head (75f1223).

Files with missing lines Patch % Lines
src/pytest_bdd/gherkin_parser.py 84.44% 3 Missing and 4 partials ⚠️
src/pytest_bdd/parser.py 87.87% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #759      +/-   ##
==========================================
- Coverage   96.05%   95.34%   -0.71%     
==========================================
  Files          55       53       -2     
  Lines        2359     2193     -166     
  Branches      136      157      +21     
==========================================
- Hits         2266     2091     -175     
- Misses         56       60       +4     
- Partials       37       42       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

def _to_raw_string(normal_string: str) -> str:
return normal_string.replace("\\", "\\\\")
def replace_datatable_values(document: GherkinDocument) -> None:
"""Replace all cell values in DataTables within a GherkinDocument using _to_raw_string."""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this necessary? Maybe it should be done by gherkin-official?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, I'd rather avoid trying to patch datatables here at all, and instead handle it in src/pytest_bdd/scenario.py, since it's one place there, but many places here

yield self.parse_scenario(child.scenario, feature, rule)
for child in rule_data["children"]:
if "scenario" in child:
yield self.parse_scenario(child["scenario"], feature, rule) # type: ignore[typeddict-item]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should try avoiding # type: ignore. What's the issue here? Maybe we need to cast child to a specific type?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we keep testing the error messages?

Copy link
Contributor

@youtux youtux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should change gherkin-official to return classes instead of dicts now. WDYT?

@jsa34
Copy link
Collaborator Author

jsa34 commented Jan 6, 2025

maybe we should change gherkin-official to return classes instead of dicts now. WDYT?

Good point - ideally, I guess it should return the gherkin messages format (which will be dataclasses) so should wait to do this until that is resolved. I'll mark this as draft until then but you have rightly pointed out workarounds that should probably also be backported to gherkin-official.

@jsa34 jsa34 changed the title Remove Gherkin local dataclasses Draft: Remove Gherkin local dataclasses Jan 6, 2025
@jsa34 jsa34 changed the title Draft: Remove Gherkin local dataclasses Remove Gherkin local dataclasses Jan 6, 2025
@jsa34 jsa34 marked this pull request as draft January 6, 2025 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants