-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat/improve-error-messages Improve error messages from parsing #3257
base: master
Are you sure you want to change the base?
feat/improve-error-messages Improve error messages from parsing #3257
Conversation
Please edit |
tests/jq.test
Outdated
@@ -117,11 +117,11 @@ null | |||
|
|||
%%FAIL | |||
{(0):1} | |||
jq: error: Cannot use number (0) as object key at <top-level>, line 1: | |||
jq: error at line 0, column 1: Cannot use number (0) as object key at <top-level>, line 1: |
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 think line and column should be 1-origin. Like JSON parse error.
$ printf '[' | jq .
jq: parse error: Unfinished JSON term at EOF at line 1, column 1
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! will do a followup with the changes
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.
hello, @itchyny, I have made an update, but I feel uneasy with the token removals that bison did upon generation. build succeeded and I have also changed tests. I'm not sure it is enough. Thank you!
src/parser.c
Outdated
@@ -67,7 +67,7 @@ | |||
|
|||
|
|||
/* First part of user prologue. */ | |||
#line 1 "src/parser.y" | |||
#line 1 "parser.y" |
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.
You have to use ./configure --enable-maintainer-mode
flag and make
to generate parser code.
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.
regenerated with maintainer mode
tests/jq.test
Outdated
|
||
%%FAIL IGNORE MSG | ||
%::wat | ||
jq: error: syntax error, unexpected '%', expecting $end (Unix shell quoting issues?) at <top-level>, line 1: | ||
jq: error: syntax error, unexpected '%', expecting $end (Unix shell quoting issues?) at <top-level>: |
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.
Why?
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 thought initially that it was added by hand, I have changed it back
51b8c12
to
5ef6d59
Compare
src/parser.y
Outdated
if (strstr(s, "unexpected")) { | ||
#ifdef WIN32 | ||
locfile_locate(locations, *loc, "jq: error: %s (Windows cmd shell quoting issues?)", s); | ||
locfile_locate(locations, *loc, | ||
"jq: syntax error at line %d, column %d: Found %s where it was not expected. This might be due to Windows cmd shell quoting issues.", |
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.
Let's focus on adding just at line %d, column %d
in this pull request for merging with fewer objections. I don't like long error messages like this, and don't like the existing platform-dependent shell quoting issue message, but such improvement can be done after adding the line position report.
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.
updated
Suggestion for issue:
#3256
Open for improvements :)