Skip to content

Fix/tree sitter parsing expression failure #186

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

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

Conversation

helgardferreira
Copy link

@helgardferreira helgardferreira commented Mar 4, 2024

🤔 What's changed?

A helper function stripBlacklistedExpressions to help with tree-sitter's parsing in certain edge cases where parsing is currently erroneous.

⚡️ What's your motivation?

We're currently using playwright-bdd so that we can use playwright as our test runner and cucumber for BDD. Unfortunately, the language server is currently not working for the decorator syntax for playwright-bdd which can be found here.

The reason for this seems to be that the version of tree-sitter that is used by @cucumber/language-service is unable to parse TypeScript files that contain decorators for classes. This PR is an interim fix for that until either tree-sitter addresses the issue (if the issue is on their behalf - I'm not sure) or until @cucumber/language-service changes the tree-sitter parsing to work as expected.

🏷️ What kind of change is this?

  • 🐛 Bug fix (non-breaking change which fixes a defect)

♻️ Anything particular you want feedback on?

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

This text was originally generated from a template, then edited by hand. You can modify the template here.

@barrymichaeldoyle
Copy link

Is anyone going to review this? As it stands right now Helgard's unofficial cucumber extension on VSCode marketplace is the only one that works for our project.

@kieran-ryan
Copy link
Member

@helgardferreira, the here link appears to be broken. Would you be able to provide an updated link or according example?

@helgardferreira
Copy link
Author

@kieran-ryan Oh sorry, I think the docs might have gotten updated. I've updated the link which can be found here: https://vitalets.github.io/playwright-bdd/#/writing-steps/decorators

@kieran-ryan
Copy link
Member

@kieran-ryan Oh sorry, I think the docs might have gotten updated. I've updated the link which can be found here: https://vitalets.github.io/playwright-bdd/#/writing-steps/decorators

Cheers, those examples appear to work for me; is there perhaps a variation of other code combined with these decorators that's causing the issue; if any examples? Would be great to get the different cases added to the unit tests as a test data source.

image

@helgardferreira
Copy link
Author

I can definitely make examples. The current specific issue we're running into is in a proprietary codebase but I'll create an environment to demonstrate the issue 🙂

@luke-hill
Copy link
Contributor

@helgardferreira are you able to rebase this and we will commit to reviewing and merging this soon

@luke-hill
Copy link
Contributor

@michaelboyles are you able to review this potentially. I can also look into getting you the commit bit if you would like to review and merge these independently of me?

@michaelboyles
Copy link
Contributor

@luke-hill If it was an issue with tree-sitter like OP was speculating it could be, then the version has been bumped since this was opened, so it could be fixed anyway

Or if the issue is with our tree-sitter queries then we can easily change those instead of this workaround

Because we never got an example that allowed us to repro, I don't think we should merge this. I would just close for now if they are not going to respond. Can always re-open

Sure. I don't know what the process is, but if you wanna do that then feel free

@luke-hill
Copy link
Contributor

I'll wait to hear back from @helgardferreira and then go from there

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

Successfully merging this pull request may close these issues.

5 participants