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

Table formatting for full width characters #53

Merged
merged 3 commits into from
Feb 20, 2024
Merged

Conversation

kieran-ryan
Copy link
Sponsor Member

@kieran-ryan kieran-ryan commented Jan 20, 2024

🤔 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:

Screenshot 2024-01-20 at 16 50 23

🏷️ What kind of change is this?

  • 🐛 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:

  • 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.

@kieran-ryan kieran-ryan added the 🐛 bug Defect / Bug label Jan 20, 2024
@kieran-ryan kieran-ryan self-assigned this Jan 20, 2024
@kieran-ryan kieran-ryan marked this pull request as ready for review January 20, 2024 19:48
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.

This solution isn't perfect. There will be other cases where the table is visually misaligned, probably with flag emojis and such. But as a work around for the what is probably the most common problem I do think this is acceptable.

That said, as this problem is common to both implementations, please do add an example in the testdata folder.

@mpkorstanje
Copy link
Contributor

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
Copy link
Sponsor Member Author

kieran-ryan commented Feb 19, 2024

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?

Screenshot 2024-02-19 at 22 22 00

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
Copy link
Contributor

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 #59 and #62.

Feel free to merge and release this.

@kieran-ryan kieran-ryan merged commit 9ff2baf into main Feb 20, 2024
8 checks passed
@kieran-ryan kieran-ryan deleted the full-width-char-tables branch February 20, 2024 00:35
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
Development

Successfully merging this pull request may close these issues.

Tables with CJK characters do not align
2 participants