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

Got an error when equesting textDocument/codeAction #100

Closed
DouyuShinyruo opened this issue Sep 30, 2022 · 9 comments · Fixed by #108
Closed

Got an error when equesting textDocument/codeAction #100

DouyuShinyruo opened this issue Sep 30, 2022 · 9 comments · Fixed by #108

Comments

@DouyuShinyruo
Copy link

👓 What did you see?

Got an error when equesting textDocument/codeAction.
I wrote And 60 minutes have passed in my feature file and wrote @When("{int} {time_unit} have/has passed") in my *.steps.java and I hope the feature file should jump to the step definition, but it got a error.

[Info  - 10:43:53 AM] * Found 37 parameter types in those glue files
[Info  - 10:43:53 AM]   * {time_unit} = (?i)(nanosecond|microsecond|millisecond|second|minute|hour|day)s?

✅ What did you expect to see?

The feature file should jump to the step definition.

📦 Which tool/library version are you using?

Cucumber for Visual Studio Code V1.3.0

@DouyuShinyruo
Copy link
Author

And the output is

[Info  - 10:48:41 AM] * Built 449 suggestions for auto complete
[Error - 10:48:41 AM] Request textDocument/codeAction failed.
  Message: Request textDocument/codeAction failed with message: Invalid regular expression: /((?i)(nanosecond|microsecond|millisecond|second|minute|hour|day)s?)/: Invalid group
  Code: -32603 

@aslakhellesoy
Copy link
Contributor

Thanks for the detailed report! I'll look into this.

@aslakhellesoy
Copy link
Contributor

@DouyuShinyruo can you please share your Java code where you define the parameter type?

@aslakhellesoy
Copy link
Contributor

The embedded (?i) flag is Java-specific, and isn't understood by JavaScript RegExps. (The VSCode extension runs on Node.js).

If you change your expression from ((?i)(nanosecond|microsecond|millisecond|second|minute|hour|day)s?) to (nanosecond|microsecond|millisecond|second|minute|hour|day)s? it should work.

Is there a particular reason why you want it to be case insensitive?

@aslakhellesoy aslakhellesoy transferred this issue from cucumber/vscode Oct 5, 2022
@aslakhellesoy
Copy link
Contributor

aslakhellesoy commented Oct 5, 2022

I think the best we can do here is to strip away any (?i) flags from parameter type regexps. This means they would no longer be case-insensitive in the language server (and the VSCode extension), so some Gherkin steps might show up as undefined.

Using the case insensitive flag in parameter type regexps is disallowed in the Ruby and JavaScript implementations of Cucumber Expressions:

https://github.com/cucumber/cucumber-expressions/blob/abc7d23299acc5d80de1ebd8fda94ab39015de3c/javascript/test/ParameterTypeTest.ts#L7-L12

https://github.com/cucumber/cucumber-expressions/blob/abc7d23299acc5d80de1ebd8fda94ab39015de3c/ruby/spec/cucumber/cucumber_expressions/parameter_type_spec.rb#L6-L12

The reason for this is that the //i flag is global. See cucumber/cucumber-expressions@63762bd

We don't have this limitation in the Java implementation of Cucumber Expressions. Do you think we should add a similar constraint there @mpkorstanje @mattwynne?

@mpkorstanje
Copy link
Contributor

mpkorstanje commented Oct 5, 2022

This is a difficult one.

It would definitely solve the problem for VS code, but also:

  • It would be a breaking change so we'd have to delay it until Cucumber JVM 8.0.
  • People can still use VS code with Cucumber JVM 7.x
  • It's kinda useful. While most people will write seconds, a few might write Seconds.
  • It feels like playing whack-a-mole. What's the next cross platform difference that we could hit?

How does VS code currently handle Java regexes? I would be surprised if we are the first.

Do we have JS libraries that support some of Javas Regex features that we could use?

Could we do a "best effort" and only strip the case insensitive pattern in VS code?

@aslakhellesoy
Copy link
Contributor

It's kinda useful

I agree. We'd have supported it in the JavaScript/Ruby implementations if (?i) was supported.

How does VS code currently handle Java regexes? I would be surprised if we are the first.

We parse them and hope for the best. We have done 0 work to make sure they work.

We do some sanitisation of Python regexps - they also have some features that are unsupported in JavaScript. See

toStepDefinitionExpression(node: TreeSitterSyntaxNode): StringOrRegExp {
const s = node.text.slice(1, -1)
// remove python named capture groups.
// TODO: This should be temporary. Python supports
// a wider array of regex features than javascript
// a singular way of communicating regex consistent
// across languages is necessary
const specialChars = /[^$<>]/
if (specialChars.test(s)) {
const replaced = s.split('?P').join('')
return new RegExp(replaced)
} else {
return s
}
},

Maybe we could have used https://github.com/jmchilton/pyre-to-regexp here.

Do we have JS libraries that support some of Javas Regex features that we could use?

I found this:

I think the simplest solution for now is to just strip away any (?i).

@leibson
Copy link

leibson commented Jan 23, 2024

I see that this is closed, but I have a similar problem, but I am using C#, which should allow the following decorator:
[Then(@"(?i)Check in db the Tbl_Cards:")]

Failed to reindex: Invalid regular expression: /(?i)Check in db the Tbl_Cards:/: Invalid group
Should I create a new ticket?

@kieran-ryan
Copy link
Member

Thanks for catching @leibson - yes a new ticket would be great! 🙌

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 a pull request may close this issue.

5 participants