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

Improve Gherkin support #3643

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Improve Gherkin support #3643

wants to merge 7 commits into from

Conversation

Hirse
Copy link
Contributor

@Hirse Hirse commented Oct 22, 2022

Resolves #3638
Resolves #3639

image

Changes

  • Move keywords to mode to support keywords with spaces and those ending with a colon
  • Update list of keywords (see below)
  • Change * from symbol to keyword since that is how it's described in the reference
  • Tighten variable definition to disallow whitespace
  • Only recognize keywords, comments, tags, and docstrings at the beginning of a line
  • Support docstrings starting with triple backticks
  • Remove (non-docstring) string mode

Reference

Checklist

  • Added markup tests
  • Updated the changelog at CHANGES.md

@pkufranky
Copy link

thanks very much

@joshgoebel
Copy link
Member

Technically all features (except variables) are only valid at the beginning of a line.

Can't we do this with multi-match and ^ (start line)? Just use the first part of the match to grab the whitespace...

@joshgoebel
Copy link
Member

and those ending with a colon

Is the colon part of the keyword though?

@Hirse
Copy link
Contributor Author

Hirse commented Oct 27, 2022

Updated with a new version that removes the custom keyword pattern and fixes false positives.

I couldn't figure out a good way to match for "beginning of line, skipping the whitespace" as /^[ \t]*/ triggers the "zero-width regex" error so there's currently a bunch of duplication.

scope: 'string'
},
{
begin: /^[ \t]+/,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This repetition is frustrating. Could a single top-level rule to eat tabs at the start of lines accomplish this same thing (probably would leave to false positives)? If not why not add it to each multi-match rule as an optional first component:

        begin: [
          LINE_BEGINS_WS,
          /(Feature|Rule|Examples?|Scenario(?:s| Outline| Template)?|Background)/,
          /:/
        ],
        beginScope: {
          2: 'keyword',
          3: 'punctuation',
        },

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, eating whitespace of the beginning of the line with an empty mode seems to work. Is that what you had in mind? 😄

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The multi-match method I show above is what I had in mind, yes. That's the best way I know to handle this kind of thing (without putting the whitespace inside the highlight zone).

@pkufranky
Copy link

Any progress for this pr?

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

Successfully merging this pull request may close these issues.

(gherkin) Rule keyword is not highlighted (gherkin) highlight wrongly for unclosed '<'
3 participants