-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fix: Added a function to improve jscript prefix identifier #305
base: master
Are you sure you want to change the base?
Conversation
@philhaworteks I have built this minor fix on top of your work in #280. We can merge when your changes are approved. |
Even my intial request is not working with java 8 due to java.util.Optional.or that was provided only from java 9. |
I used java 9 to get the Optional code to work. I changed the code to the following:
It doesn't look pretty but it works. |
I'm not sure this is worth it as this solution works fine:
|
Its a minor change that added additional tolerance to detect inline javascript. Seems much cleaner to use regex to determine the JS engine from the prefix. I made additional changes in the most recent commit that adds further tolerance to detect a variety of cases.
Other use case 1:
Other use case 2:
Other use case 3:
Other use case 4:
Or any other variants that might be occur. |
@rouazana If this seems unnecessary feel free to close off the issue. Found the lack of JS engine detection a minor inconvenience personally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not against it as it can improve this piece of code, but I guess there are currently some issues.
be9da68
to
d2e560a
Compare
2c11add
to
aeec6a3
Compare
@rouazana I have implemented some tests and ameliorated the pattern compilation to reduce the overhead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for taking my comments into account!
I have only 2 remaining suggestions:
- Try to name your tests with a meaningful function name, for example
scriptShouldEvaluateWhenStartingWithANewLine
fortestJsPrefix1
- I'm not sure you are really testing what I mean, for example that the right JS engine is chosen if the
js:
orrjs:
strings appear in different places in the expression.
When using the following prefix to scripts such as
rjs
or others:The following error is thrown:
This occurs due to the prefix containing the following:
The
.contains
method in Maps object is unable to match this pattern.This functionality works well with the following script:
As the script is done in-line the
.contains
method is able to match and find the appropriate jscript evaluator.I have added a new function that uses pattern matching with the help of a small regex function which should resolve the above described issue.
I have tested this in scenarios where no script engine has been defined without posing any issue.
Also I have built on top of the work in #280 to seamlessly merge this minor fix.