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

Fix error position property for errors in functions #570

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

asennikov
Copy link

@asennikov asennikov commented Apr 12, 2022

While playing with the JSONata exerciser, I've noticed that the position property gets set correctly for all of the errors I was able to reproduce, except for everything related to calling functions, see a couple of examples below.

looks good:

image

looks not good:

image

image

It seems like due to how JSONata evaluates functions, the position is already moved by 1 to the opening bracket character, which messes up the error highlighting, as the error.token does not include the opening ( character, but the position is already past it.

IMHO, it would be better if JSONata was reducing the position by 1 to match the reported token.

This PR is aiming to implement this adjusted behavior, but if there's a better way to solve this problem - I'm open to feedback!

@coveralls
Copy link

coveralls commented Apr 12, 2022

Coverage Status

Coverage remained the same at 100.0% when pulling 305102b on asennikov:asennikov/fix-error-position-on-function-failures into 3cea53f on jsonata-js:master.

@asennikov asennikov changed the title Fix error position property set for errors in functions Fix error position property for errors in functions Apr 13, 2022
@mattbaileyuk
Copy link
Member

mattbaileyuk commented May 4, 2022

Firstly, thanks for the PR!

However, I am concerned that this would be a breaking change, altering the behaviour for anyone who has been an interface that relies on the current error output; the exerciser is just one example here, and I suspect this might impact IBM App Connect (where JSONata originated), and I'm aware there are a number of UIs now built which use the language.

I'm not saying we shouldn't make this change, just that it might need some caution/major version bump.

@asennikov
Copy link
Author

the exerciser is just one example here

yeap, that's fair - I mostly used exerciser as an easy example to reproduce the issue, but I'm actually more interested in fixing it for the product I'm working on which exposes JSONata to our users and displays these errors.

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.

3 participants