ducminh-phan / reformat-gherkin

Formatter for Gherkin language
MIT License
22 stars 13 forks source link

newline "\n" in table causes reformatter to fail #51

Closed conradogarciaberrotaran closed 2 years ago

conradogarciaberrotaran commented 2 years ago

Describe the bug

When trying to reformat a feature file with new lines in the example tables, the reformatter fails with the following output:

Parser errors:
(11:2): expected: #EOF, #TableRow, #TagLine, #ExamplesLine, #ScenarioLine, #ScenarioOutlineLine, #Comment, #Empty, got 'and here |'
(10:7): inconsistent cell count within the table
Please report a bug on https://github.com/ducminh-phan/reformat-gherkin/issues.
This invalid output might be helpful:
/tmp/rfmt-ghk_lhk5a0n5.log

All done! 💥 💔 💥
1 file failed to reformat.

To Reproduce

Steps to reproduce the behaviour

Having a file like this

test.feature:

Feature: Breaking gherkin reformatter
  Scenario Outline: 
    Given I want to cause a bug
    When I try to reformat with an example table with a message like <message>
    Then gherkin reformatter explodes

    Examples:
      | message                   | 
      | newline here\n and here\n | 

run reformat-gherkin test.feature

Expected behaviour

File is reformatted

Actual behaviour

Reformatter exits

Environment

Found this tool today, and I think it's awesome. Thank you for maintaining it. Hope we can fix this issue.

Cheers.

ducminh-phan commented 2 years ago

Looks like we have an issue similar to this one: #21. The solution is simply to replace \n (newline character) in the parsed cell value with \\n (a \ followed by an n).

However, the version of gherkin-official that we are using (4.1.3) parses the last table cell value as newline here\n and here, note that the last \n is missing. The latest version of gherkin-official (22.0.0) correctly parses the cell value as newline here\n and here\n.

Upgrading gherkin-official to 22.0.0 will be a breaking change, and we need to update the parsing logic as well since the official parser produces the result in a different format.

So I would say that we fix the issue by replacing \n with \\n in the parsed cell value, and accept that the last \n will not be there after formatting. Version 3.0.0 will be released with the latest version of gherkin-official.

What do you think? @conradogarciaberrotaran @rayjolt

Also, @rayjolt could you take a look at the discussion that we had in #32? Looks like cucumber/cucumber#865 was fixed.

conradogarciaberrotaran commented 2 years ago

I think it's a good idea. I really want to add this as a pre-commit in all my projects, but this issue is blocking me. Is there a way to make the formatter ignore a scenario?

Thanks

rayjolt commented 2 years ago

@ducminh-phan Yes, that sounds like a good way to fix things. I just checked out gherkin-official 22.0.0, and it looks like upgrading will be quite a bit of work. So fixing this issue imperfectly using gherkin-official 4.1.3, then upgrading later seems like the appropriate thing to do.

rayjolt commented 2 years ago

As for #32, we should make issues for upgrading to gherkin-official 22.0.0 and for fixing the nested docstring edge case so that we can keep track of things. I've run out of time for that today, but maybe tomorrow.