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

python: provide informative exception for trailing escapes in tables #241

Merged
merged 5 commits into from Apr 13, 2024

Conversation

kieran-ryan
Copy link
Sponsor Member

@kieran-ryan kieran-ryan commented Mar 31, 2024

πŸ€” What's changed?

With the Python implementation:

  1. Whitespace either side of a table row is stripped.
  2. The table row is passed to a function to strip and yields its cell values
  3. Characters in each cell are iterated over to handle them appropriately, such as new columns (|), escape characters, etc.
  4. As escape characters are typically followed by a special character (e.g. n, t), when an escape character is detected \\ an additional next call is made to grab the next character
  5. As in step 1, whitespace after a trailing escape character (\\) would be stripped, there would not be a following character and a StopIteration error is raised.

def split_table_cells(self, row):
"""
An iterator returning all the table cells in a row with their positions,
accounting for escaping.
"""
row = iter(row)
col = 0
start_col = col + 1
cell = ''
first_cell = True
while True:
char = next(row, None)
col += 1
if char == '|':
if first_cell:
# First cell (content before the first |) is skipped
first_cell = False
else:
yield (cell, start_col)
cell = ''
start_col = col + 1
elif char == '\\':
char = next(row)

By defaulting the iterator to an empty string, when there is a trailing escape character, the trailing escape character will not trigger a StopIteration error and subsequent code will appropriately raise an inconsistent cell count error message.

⚑️ What's your motivation?

🏷️ What kind of change is this?

  • πŸ› Bug fix (non-breaking change which fixes a defect)

♻️ Anything particular you want feedback on?

πŸ“‹ Checklist:

  • I agree to respect and uphold the Cucumber Community Code of Conduct
  • I've changed the behaviour of the code
    • I have added/updated tests to cover my changes.
  • Users should know about my change
    • I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

@kieran-ryan kieran-ryan added the πŸ› bug Defect / Bug label Mar 31, 2024
@kieran-ryan kieran-ryan self-assigned this Mar 31, 2024
@kieran-ryan kieran-ryan marked this pull request as ready for review March 31, 2024 18:20
Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

Cheers!

python/gherkin/gherkin_line.py Show resolved Hide resolved
python/test/gherkin_test.py Show resolved Hide resolved
Copy link
Sponsor Member Author

@kieran-ryan kieran-ryan left a comment

Choose a reason for hiding this comment

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

Thanks for the review! Will go with your latest preference following the considerations I have added

python/test/gherkin_test.py Show resolved Hide resolved
python/gherkin/gherkin_line.py Show resolved Hide resolved
Copy link
Contributor

@mpkorstanje mpkorstanje left a comment

Choose a reason for hiding this comment

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

Alright, I had a look at the Javascript and Ruby implementations and they're just as confusing. Java was the odd one out. So this is a good solution.

What is weird though, they should have the same problem because both JS and Ruby do something equivalent to:

        elsif char == '\\'
          char = row[col]

And that should result in char being undefined. Which is rendered to string as undefined in JS atleast.

"hello " + "cucumber"[8] 

"hello undefined"

Fixing this it outside the scope of this PR though.

@mpkorstanje
Copy link
Contributor

Not a problem in JS, while we do append undefined, this cell is never used because it is not terminated by a |.

image

@mpkorstanje mpkorstanje changed the title Provide appropriate exception for inconsistent table cells with trailing escape Python: throw informative exception for trailing escapes in tables Apr 13, 2024
@mpkorstanje mpkorstanje changed the title Python: throw informative exception for trailing escapes in tables python: throw informative exception for trailing escapes in tables Apr 13, 2024
@mpkorstanje mpkorstanje changed the title python: throw informative exception for trailing escapes in tables python: informative exception for trailing escapes in tables Apr 13, 2024
@mpkorstanje mpkorstanje changed the title python: informative exception for trailing escapes in tables python: provide informative exception for trailing escapes in tables Apr 13, 2024
@mpkorstanje mpkorstanje merged commit 0353c9d into main Apr 13, 2024
4 checks passed
@mpkorstanje mpkorstanje deleted the fix-py-table-escape branch April 13, 2024 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
πŸ› bug Defect / Bug
Projects
None yet
2 participants