ducminh-phan / reformat-gherkin

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

--alignment should not force a space between the step keyword and the statement #61

Open kdelvare opened 2 years ago

kdelvare commented 2 years ago

When formatting french gherkin files, the added space breaks the scenario (step becomes undefined).

Example: Quand il clique sur le bouton de suppression Et qu'il confirme la popup de validation

Et qu'il should not be reformatted to Et qu' il (Quand = When, Et que / Et qu' = And)

Either allowing no space, or adding a new option value for --alignment would be fine.

ducminh-phan commented 2 years ago

Looks like a bug to me. The official Gherkin parser already has information on whether there should be a space after the step keyword (example). However, in reformat-gherkin, when building the AST, step keywords are trimmed, then the formatter always add an extra space after the step keyword.

@rayjolt Can you confirm the similar behaviour with Japanese? To be specific, this test data file should have no spaces after step keywords, correct?

rayjolt commented 2 years ago

In an ideal world, there should be no spaces after the Japanese step keywords (this is how Japanese text works in real life). However, if there is no space then some tools will not recognise the words as step keywords. One such tool that I can name off the top of my head is Froglogic Squish. This means that the test data file is correct, at least for Squish.

So it seems that we are in a situation in which some keywords should have a following space, and some keywords should not; and in which different tools may or may not require a space for keywords in a given language. To solve this, I would use the Gherkin parser's language data to get the default spacing for each keyword, and then make the decision of whether to add a trailing space configurable.

I can think of three possible levels of configuration to consider:

  1. A blanket setting (add or remove trailing spaces for every keyword in every file)
  2. Per language (like the blanket setting, but can be changed independently for each language)
  3. Per keyword (customise the trailing spaces for each possible keyword in each language)

To know which of these is a good idea, I think we need more data about how different tools format different language keywords. For example, maybe there is a tool that recognises Et qu' il in French, but doesn't recognise Et qu'il. If this is the case, it would point to needing per-keyword configuration so that users of that tool can work around this. If not, maybe we can get away with just having a blanket setting (this would be enough to use Japanese Gherkin files in Squish).

rayjolt commented 2 years ago

I just tested how Squish 3.7.0 deals with Et qu'il confirme la popup de validation in French. It treats the keyword as Et, and the step definition must be qu'il confirme la popup de validation. It doesn't accept a step definition of il confirme la popup de validation. This behaviour is incorrect, but doesn't affect our configuration choices. I'll have a look for some other examples.

rayjolt commented 2 years ago

I checked Lorsqu', and Squish 3.7.0 accepted both Lorsqu'il confirme la popup de validation and Lorsqu' il confirme la popup de validation. So it seems Squish is correct here, and this doesn't affect our configuration choices.

rayjolt commented 2 years ago

Hm, interesting. It seems that Squish is now parsing Japanese step names correctly. I tried 前提として何かの条件がある, and Squish 3.7.0 correctly parsed the keyword as 前提. It must be that the issue requiring a space was an error in an earlier version of Squish that has since been fixed. So for using Squish with Japanese, we won't even need a blanket setting to change the trailing spaces unless we want to support older versions.

Perhaps the best course of action is to use the Gherkin parser's language data to get the spacing after each keyword, and then only make the spacing configurable if we hear that this is necessary from any users.

rayjolt commented 2 years ago

I added some tests for trailing space behaviour to my fork. They are failing at the moment - I will write some code to fix these and then create a pull request.

@kdelvare I added the following tests for French. Does the expected output look OK to you?

At the moment, the expected output for --alignment left looks like this:

Scénario: Suppression des articles du panier
  Étant donné qu'un utilisateur consulte les articles de son panier
  Lorsqu'        il clique sur le bouton de suppression
  Et qu'         il confirme la popup de validation
  Alors          l'article est supprimé

This is not great, as it splits up "Lorsqu'il" and "Et qu'il". However, I can't think of a better way of doing it that would keep the step keywords left-aligned and the step names aligned with each other.

If you have better suggestions, please let me know. Also, feel free to correct my lousy French. ;)

kdelvare commented 2 years ago

This seems perfect to me, and I see no issue with splitting up "Lorsqu'il" and "Et qu'il", when the user asks for left alignment.

And your french is perfect ;)