-
Notifications
You must be signed in to change notification settings - Fork 6
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 more tests for selectors #90
Add more tests for selectors #90
Conversation
}, | ||
{ | ||
"name": "minus space", | ||
"selector": "$[- 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 an argument could be made for this to be put in the whitespace tests as well, but I can see it staying here.
Other opinions welcome.
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 getting more and more inclined to think we can benefit from tags. I might be able to make an attempt to add tags in a day or two.
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.
Yeah, I think a lot of these in particular could be categorized as just "parsing". I don't think that it's practical to have 2^53 items in a JSON file 😆
}, | ||
{ | ||
"name": "minus space", | ||
"selector": "$[:- 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.
Same as with the index variant of this test.
@@ -610,6 +610,28 @@ | |||
9 | |||
] | |||
}, | |||
{ |
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.
Would it make sense to add similar checks for the "start" and "step" parameters?
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.
Agreed. Let's go ahead and add those tests, then this should be good.
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 have duplicated them now for start and step, please let me know if it is ok like this.
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, let's expand the int boundary tests.
Thanks again @Marcono1234! |
}, | ||
{ | ||
"name": "plus", | ||
"selector": "$[+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.
Why is this invalid ? in Java, this is valid.
private def `int`[_: P]: P[Int] = P("0" | (CharIn("+\\-").? ~ `DIGIT1` ~ `DIGIT`.rep)).!
.map(_.toLong)
.filter(value => MIN_INTEGER < value && value < MAX_INTEGER)
.map(num => Math.min(Int.MaxValue, Math.max(Int.MinValue, num)).toInt)
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 standard does not allow +
for integers.
index-selector = int ; decimal integer
int = "0" /
(["-"] DIGIT1 *DIGIT) ; - optional
DIGIT1 = %x31-39 ; 1-9 non-zero digit
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.
Oh, I see. It seems I missed this, but I think +
can also be optional, will update my implementation, thanks
The "min exact" and "max exact" values here are based on the "range of exact integer values defined in Internet JSON (I-JSON)", as required by the JSONPath specification for index and slice selectors.
I have kept the existing overflow tests because they might still be useful to test overflow during parsing, which might happen before the "range of exact integer values" check in JSONPath implementations.
To be safe, could you please check how your JSONPath implementations behave, to make sure I did not overlook something.