ducminh-phan / reformat-gherkin

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

Step keywords are always aligned to a width of five characters when specifying an alignment #27

Closed rayjolt closed 4 years ago

rayjolt commented 4 years ago

When using --alignment left or --alignment right, step keywords are always aligned to a width of five characters. This is correct for English Gherkin documents where all scenarios contain the "Given" keyword, but is incorrect for scenarios that don't contain the "Given" keyword, and for non-English documents containing scenarios where the longest keyword is not five characters long.

Consider the following feature with no "Given" keyword.

Feature: Feature name

  Scenario: Scenario name
    When I do something
    Then I get a result

Reformatting this with the --alignment left option gives the following:

Feature: Feature name

  Scenario: Scenario name
    When  I do something
    Then  I get a result

This has two spaces after "When" and "Then", but there should only be one space after each.

Also, consider this Czech-language feature.

# language: cs

Požadavek: Feature name

  Scénář: Scenario name
    Za předpokladu a given step
    Když a when step
    Pak a then step

When this is reformatted with the --alignment left option, you get the following:

# language: cs

Požadavek: Feature name

  Scénář: Scenario name
    Za předpokladu a given step
    Když  a when step
    Pak   a then step

The "Když" and "Pak" steps should be aligned to the width of the "Za předpokladu" step, but instead they are aligned to five characters.

There are two reasons for this:

  1. The Gherkin dialect isn't passed to generate_step_line in LineGenerator.visit_step.
  2. The maximum keyword length is calculated from all possible keyword lengths, instead of only the keywords in the current scenario.

Python version: 3.8.1 Reformat-gherkin version: latest develop (75efa435e745b9ccb09ef170d5cfbd69f8d91807)

rayjolt commented 4 years ago

I think the fix should be to only look at the keywords in the current scenario when calculating the maximum length. This way we don't have to worry about getting the keywords for the correct dialect from the gherkin module - we can just use the ones in the AST. However, this does mean we have to keep track of the keyword widths ourselves. Probably the best way would be to add the step nodes to the context map.

rayjolt commented 4 years ago

I'll have a go at implementing the fix for this. Let me know if you think another approach would be better.

ducminh-phan commented 4 years ago

When using --alignment left or --alignment right, step keywords are always aligned to a width of five characters. This is correct for English Gherkin documents where all scenarios contain the "Given" keyword, but is incorrect for scenarios that don't contain the "Given" keyword, and for non-English documents containing scenarios where the longest keyword is not five characters long.

Consider the following feature with no "Given" keyword.

Feature: Feature name

  Scenario: Scenario name
    When I do something
    Then I get a result

Reformatting this with the --alignment left option gives the following:

Feature: Feature name

  Scenario: Scenario name
    When  I do something
    Then  I get a result

This has two spaces after "When" and "Then", but there should only be one space after each.

Also, consider this Czech-language feature.

# language: cs

Požadavek: Feature name

  Scénář: Scenario name
    Za předpokladu a given step
    Když a when step
    Pak a then step

When this is reformatted with the --alignment left option, you get the following:

# language: cs

Požadavek: Feature name

  Scénář: Scenario name
    Za předpokladu a given step
    Když  a when step
    Pak   a then step

The "Když" and "Pak" steps should be aligned to the width of the "Za předpokladu" step, but instead they are aligned to five characters.

There are two reasons for this:

  1. The Gherkin dialect isn't passed to generate_step_line in LineGenerator.visit_step.
  2. The maximum keyword length is calculated from all possible keyword lengths, instead of only the keywords in the current scenario.

Python version: 3.8.1 Reformat-gherkin version: latest develop (75efa43)

I agree that reason 1 is a bug. However, reason 2 is a deliberate decision I made, so that the texts of the steps are aligned vertically if an alignment mode is specified.

rayjolt commented 4 years ago

I agree that reason 1 is a bug. However, reason 2 is a deliberate decision I made, so that the texts of the steps are aligned vertically if an alignment mode is specified.

I see - aligning all the steps in the document does sound like a good idea, and I agree that we should keep that behaviour. There is a problem for languages that have multiple translations of one step keyword, though. For example, in German there are three different ways of writing "Given": "Angenommen", "Gegeben sei", and "Gegeben seien". With the current approach, you always get the padding for "Gegeben seien" (13 letters), even if you only ever use "Angenommen" (10 letters) in the document. This gives you three extra padding spaces, which I think looks unnatural.

How about finding the longest step keyword used in the document, and then aligning all the steps based on that? That would keep the text of all the steps in the document aligned vertically, and would prevent extra padding spaces in languages with multiple translations of step keywords.

ducminh-phan commented 4 years ago

How about finding the longest step keyword used in the document, and then aligning all the steps based on that? That would keep the text of all the steps in the document aligned vertically, and would prevent extra padding spaces in languages with multiple translations of step keywords.

Aligning all the steps based on the longest step keyword sounds like a good idea to me.