-
Notifications
You must be signed in to change notification settings - Fork 41
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
Single quoted strings #39
base: master
Are you sure you want to change the base?
Single quoted strings #39
Conversation
"github.com/google/go-cmp/cmp" | ||
"gopkg.in/yaml.v3" | ||
) | ||
|
||
var knownParsingErrors = map[string]string{ | ||
`bracket_notation`: `parsing error: $['key'] :1:3 - 1:8 could not parse string: invalid syntax`, |
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.
Please explain why all this test cases are removed.
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 removed expected exceptions are those, that should no longer appear or should lead to a different error than what was listed. Partially due to the changes made by this PR, but we realized that some other test cases no longer held true either due to previous changes by other PRs. These changes should update the list to match the latest expected results.
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.
Okay got it, thank you.
Would you please be so kind to download newest version of regression_suit.yaml and adapt it to you 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.
While it's most likely a sensible idea, this seems out of scope for this Pull Request. The exceptions were adapted to make sure the changes are compatible. Readjusting the entirety of the regression suit however may run into even more errors and problems unrelated to the changes made, which would require additional work to fix. As such, it's probably a better idea to handle this within another issue and related PR.
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.
@Paessler-Stefan do have everything to integrate this and the gval pull-request?
To recap the situation regarding your questions so far:
- We re-enabled tests that now run as they should, because of the fixed single quote behavior.
- There was one new error uncovered. It was there previously and it is unrelated to the single quote behaviour. So to make the test suit run again, we disabled this one.
- We re-enabled unrelated tests that hat been fixed by other developments after they had been disabled as a courtesy.
- Downloading https://github.com/cburgmer/json-path-comparison/blob/master/regression_suite/regression_suite.yaml and updating all tests with it is a different kind of work, that is likely to reveal other, unrelated problems. And we consider it out of scope.
Solves #38.
This PR allows JSONPath to evaluate single-quoted strings if PaesslerAG/gval#98 is merged.
Tests have been adjusted to match new behaviour as well.