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

Bugfix/regex regression #119

Merged
merged 10 commits into from
Dec 8, 2022
Merged

Bugfix/regex regression #119

merged 10 commits into from
Dec 8, 2022

Conversation

mrkaiser
Copy link
Contributor

@mrkaiser mrkaiser commented Nov 29, 2022

🤔 What's changed?

⚡️ What's your motivation?

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, documentation etc. without changing behaviour)
  • 🐛 Bug fix (non-breaking change which fixes a defect)
  • ⚡ New feature (non-breaking change which adds new behaviour)
  • 💥 Breaking change (incompatible changes to the API)

♻️ 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.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • 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.

mrkaiser and others added 4 commits November 28, 2022 20:35
Regression on checking for the correct symbols in a regex.
Regression on checking for the correct symbols in a regex.
@mrkaiser
Copy link
Contributor Author

@aslakhellesoy This fixes the regression reported in cucumber/vscode#125

@aslakhellesoy
Copy link
Contributor

Great. Please add a test that would have failed before this change, and passes with it.

@mrkaiser
Copy link
Contributor Author

mrkaiser commented Dec 6, 2022

@aslakhellesoy apologies for the delay i have added a pythonLanguage.test.ts file

Copy link
Contributor

@aslakhellesoy aslakhellesoy left a comment

Choose a reason for hiding this comment

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

I'm merging this. I think a more robust implementation would use the same technique as in https://github.com/cucumber/language-service/blob/main/src/language/tsxLanguage.ts, where we look at the node type to decide how to extract the value, rather than looking at the node text.

But this can be done as a separate refactoring.

@@ -8,7 +8,7 @@ export const pythonLanguage: Language = {
case 'string': {
return stringLiteral(node)
}
case 'concatednated_string': {
case 'concatenated_string': {
Copy link
Contributor

Choose a reason for hiding this comment

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

I fixed this typo and nothing broke, so I assume this path isn't tested.

@aslakhellesoy aslakhellesoy merged commit 3d01665 into main Dec 8, 2022
@aslakhellesoy aslakhellesoy deleted the bugfix/regex_regression branch December 8, 2022 11:25
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.

2 participants