-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add test cases for whitespace #16
Conversation
This test will catch implementation bugs related to whitespace. jmespath/jmespath.js#58
tests/basic.json
Outdated
{ | ||
"expression": "foo\t.\tbar\t.\tbaz", | ||
"result": "correct" | ||
}, |
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.
Trailing whitespace here.
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.
Are you talking about the code or the test case?
Do you want the test expression to have a trailing \t
?
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.
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.
Should have said: "Please can you remove the trailing space characters on this line". Sorry!
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.
The one time I don't use my own IDE, 20% of LOC contain errors =D
tests/basic.json
Outdated
{ | ||
"expression": "foo\r.\rbar\r.\rbaz", | ||
"result": "correct" | ||
}, |
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.
and here
tests/basic.json
Outdated
{ | ||
"expression": "foo\r\n.\r\nbar\r\n.\r\nbaz", | ||
"result": "correct" | ||
}, |
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.
and here
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.
Looks good. My comments seem to have not hidden themselves, but they are no longer valid.
I know this is super, super late for feedback, but I won't be able to take this change. While most of the implementations ignore some degree of whitespace at the lexing stage, the grammar for jmespath uses ABNF which is strict about requiring whitespace to be explicitly modeled. From Section 3.1 of RFC 4234:
If we're going to support whitespace around expressions we would need to update the spec/grammar first, and I'm not sure at this point it's something we'd want to do (though I'm open to a discussion about it). |
This test will catch implementation bugs related to whitespace.
jmespath/jmespath.js#58