Table formatting for full width characters

kieran-ryan commented 5 months ago

🤔 What's changed?

Count full-width characters as 2 spaces to determine table column width for formatting; with half-width characters counted as 1 space, as per existing implementation.

Implementation aligns with vscode-markdown and Cucumber Autocomplete extension.

⚡️ What's your motivation?

Fixes cucumber/vscode#99.

Present formatted example:

Feature: Full width characters

  Scenario Outline: Format tables with full width characters
    Given a step table with full width characters
      | 路      | bar |
      | 路路路路路路 |  2步 |
      | 路路路    |  2步 |

Amended formatted example

Note: Appearance varies depending on font support with browsers or systems

Feature: Full width characters

  Scenario Outline: Format tables with full width characters
    Given a step table with full width characters
      | 路           | bar |
      | 路路路路路路 | 2步 |
      | 路路路       | 2步 |

Formatted example using a full-width character supported font

With the aforementioned 'amended formatted example' and the Iosevka font, it appears as follows in the browser...

Screenshot 2024-01-20 at 16 50 35

...and as follows in VSCode:

🏷️ What kind of change is this?

:bug: Bug fix (non-breaking change which fixes a defect)

♻️ Anything particular you want feedback on?

Whether should be classed as a 'bug' or an 'enhancement'.

📋 Checklist:

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

mpkorstanje commented 4 months ago

Just noticed the Java implementation doesn't test against the testdata set. Still it would be good to add an example there, so if/when we do add tests for that, it will be covered.

kieran-ryan commented 4 months ago

My present understanding is that testdata only validates that: parsing - before and after formatting - yields the same messages representation - excluding the formatting itself. To check: modify the formatting of a testdata feature file, and run npm run test inside the javascript directory; it should pass.

Thus, surplus to existing feature files, adding my example would only really test that CJK characters can be parsed and are not removed by formatting; not the formatting itself - not sure whether that is valuable?

The fix is trivial: adding assert.strictEqual(gherkinSource, formattedGherkinSource) - which fails for every feature file as they are unformatted - and of course to reformat the testdata feature files. Though unsure is that best course of action?

Interested to get your perspective on best way forward - and whether should be completed as separate issue:

[ ] Leave existing testdata untouched to validate messages representation before and after formatting. Consider git submoduling gherkin to reference its testdata without duplicating here - which may have tradeoffs.
[ ] Reformat the testdata and migrate unit tests into this testdata. Consider whether .ndjson and .tokens representations are required or can be removed.
[ ] Introduce new formatting testdata directory and migrate Java and Javascript test suites to eliminate duplication.
[ ] Something else?

mpkorstanje commented 4 months ago

Okay that's more complicated than expected. I was expecting this project to be in a better state then it is.

Let's offload those concerns to https://github.com/cucumber/gherkin-utils/issues/59 and https://github.com/cucumber/gherkin-utils/issues/62.

Feel free to merge and release this.

cucumber / gherkin-utils