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

Syntax highlighting for comments #212

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

kieran-ryan
Copy link
Member

@kieran-ryan kieran-ryan commented May 19, 2024

🤔 What's changed?

  • Syntax highlighting for comments (#)

The Language Server Protocol semantic tokens specification accepts non-negative integers for relative text positions by default. As the gherkin-util's document walker processes table rows before comments, comments can have negative relative positions and break syntax highlighting of some tokens. As such, post-processing has been introduced to correct relative positions to always be positive.

Before

Screenshot 2024-05-19 094023

After

Screenshot 2024-05-19 094111

⚡️ What's your motivation?

  • Match other common language implementations of providing syntax highlighting for comments

  • Match common syntax highlighting for gherkin (such as the Cucumber docs - see below)

    image

🏷️ What kind of change is this?

  • ⚡ New feature (non-breaking change which adds new behaviour)

📋 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 self-assigned this May 19, 2024
@kieran-ryan kieran-ryan added the ⚡ enhancement Request for new functionality label May 19, 2024
@kieran-ryan kieran-ryan marked this pull request as ready for review May 19, 2024 16:47
@kieran-ryan kieran-ryan changed the title feat: Syntax highlighting for comments Syntax highlighting for comments May 19, 2024
@michaelboyles
Copy link
Contributor

michaelboyles commented Jun 19, 2024

Nice feature, but I think you have the wrong approach with sorting the data.

As part of the walk step, you already have the absolute line number in makeLocationToken, which then gets converted to relative, to be pushed onto the array. Now you convert it back to absolute again, then back to relative.

It's like hokeypokey, back and forth. Maybe it's fine, but I worry that the performance can be better.

walkGherkinDocument already supports a custom accumulation type - it doesn't have to be number[]. I think it would be clearer to build a different kind of accumulation, then finalize that into the number[] at the end

I'll try to come up with something to show what I mean

Edit: PR for refactor: #220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚡ enhancement Request for new functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants