Skip to content

Conversation

@markborkum
Copy link
Contributor

@markborkum markborkum commented Sep 16, 2018

Copy files from https://github.com/markborkum/pacifica-jsonpath@jsonpath2 and merge conflicts.

Description

n\a

Issues Resolved

Fixes #10

Check List

  • All tests pass.
  • New functionality includes testing.
  • New functionality has been documented in the README if applicable

@markborkum markborkum requested a review from dmlb2000 September 16, 2018 23:52
@dmlb2000
Copy link
Member

@markborkum Are you ready for me to start making changes? or are you trying to get the pipeline to pass?

@markborkum
Copy link
Contributor Author

@dmlb2000 Yes, please. If you can get it to "test", I'll fix the "lint" warnings/errors.

@dmlb2000
Copy link
Member

@markborkum The linting will force module renaming, it's important to get the lint stage fixed first.

@dmlb2000
Copy link
Member

@markborkum There is a global pre-commit exclude directive for the parser module. Then you can add a README.md there describing how to build that python files?

@dmlb2000
Copy link
Member

dmlb2000 and others added 6 commits September 17, 2018 13:53
Signed-off-by: David Brown <dmlb2000@gmail.com>
Flake8 warned that the operator callables were globals. In a previous commit, the operator callables were refactored into method-locals in the operator's constructor. This is sufficient to resolve the flake8 warnings, but introduces a bug: since callables are created per instance, they have different memory locations, and hence, are not compatible with the `Node`-level definition of `__eq__`, which naively compares each `__dict__` of each operand. The fix is to refactor the operator callables into static methods.
Signed-off-by: David Brown <dmlb2000@gmail.com>
Signed-off-by: David Brown <dmlb2000@gmail.com>
Copy link
Contributor Author

@markborkum markborkum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rollback this change to the grammar. Both operands being JSON literal values is invalid syntax. For a binary operator, the left-hand operand must be a JSONPath and the right-hand operand must be a JSON value.

@dmlb2000
Copy link
Member

The reason for adding that was when I was looking at other implementations they had some weird expressions.

https://github.com/json-path/JsonPath/blob/master/json-path/src/test/java/com/jayway/jsonpath/FilterTest.java

If you look at some of the tests they have in this test... there's a couple of things.

  • The keys in the json object have dash '-' which isn't allowed in javascript, so not sure what's going on there.
  • They are testing paths like $[?(1 = '1')] which is weird but I've seen this in other languages conditionals.

markborkum and others added 4 commits September 18, 2018 09:54
In previous implementations of JSONPath, the current value is implicitly cast to array or object if the next node is an array index subscript or object index subscript. This introduces ambiguity: "is the index subscript referring to the current value, or a child value of the current value?" The solution is to use the wildcard "*" to refer to the child values explicitly.
Signed-off-by: David Brown <dmlb2000@gmail.com>
Signed-off-by: David Brown <dmlb2000@gmail.com>
@dmlb2000
Copy link
Member

dmlb2000 commented Sep 18, 2018

@markborkum Can you think of a jsonpath expression where a VariadicOperatorExpression would get created with 0 or 1 expressions passed to it?

Or is that one of those things that would need to be called by creating the object in that state?

@markborkum
Copy link
Contributor Author

@dmlb2000 The objects would need to be hand-constructed. Expressions with zero or one operand would be rejected by the parser (e.g., $[?()] and $[?(@["hello"] and)]).

@dmlb2000
Copy link
Member

@markborkum Would there be any utility in keeping those code blocks around where VariadicOperatorExpression would have 0 or 1 expressions?

@dmlb2000
Copy link
Member

dmlb2000 commented Sep 18, 2018

@markborkum Also, would array slicing like foo[:2] be allowed? Seems like it should be.

Yeah, seems like other slicing foo[::2] (grab every other element in the list) also isn't allowed. You have to specify it with numbers foo[0:2] or foo[0:-1:2] respectively.

@markborkum
Copy link
Contributor Author

@dmlb2000 There is no harm in keeping the code as it is. The implementations of the __evaluate__ and __jsonpath methods for the variadic operators are consistent with their specifications.

@dmlb2000
Copy link
Member

@markborkum Where's the spec you refer too? I'm having issues with array slice.

  • Seems the first integer is always required (I don't think it should be but I'm not finding anything that says otherwise)
  • The second colon requires the step instead of just picking a default (like 1 say)

@markborkum
Copy link
Contributor Author

@dmlb2000 The "spec" is for logical conjunction and logical disjunction in general (behavior for zero, one or more operands, etc.).

For array slice notation, the requirement for "start" is an artifact of the grammar. A "step" is always required if the second colon is present in the slice. The issue here is whether or not we are aiming for feature parity with the slice notation in an established programming language (e.g., the host language, Python).

@dmlb2000
Copy link
Member

@markborkum I'm thinking we should be similar to the host language as they've got a standard and developers will be used to that kind of array slice behavior.

markborkum and others added 4 commits September 18, 2018 14:37
Aim for feature parity with equivalent in Python programming language
@dmlb2000
Copy link
Member

dmlb2000 commented Sep 19, 2018

@markborkum The parser/__init__.py that wasn't generated by antlr, correct?

We are missing some big blocks in that file and I'm wondering if we are missing a test, exitObj() isn't getting called at all. Would we need to have a JSON object as part of an expression in a JSONPath to get that called?

Something like $[?(@["foo"] = {"bar": "baz"})]["abd"]?

@markborkum
Copy link
Contributor Author

@dmlb2000 jsonpath2/parsers/__init__.py is a hand-rolled visitor that extends the JSONPathListener class that is generated by ANTLR v4.

If the visitor is called after the parser has constructed the AST, then the majority of the raise ValueError lines are unreachable. If the visitor is called before the parser has constructed the AST then, due to back-tracking, some of the raise ValueError lines are reachable.

The JSON grammar is used for literal values in operator-based expressions. To cover the JSON grammar, we should add new tests for the equality operator, e.g., where the right-hand operand is null, true, false, a number, a string, an array, and an object.

@dmlb2000
Copy link
Member

@markborkum agreed, the raise ValueError() lines should have a # pragma: no cover with a consistent reason,

# pragma: no cover since AST already handles this case

@dmlb2000
Copy link
Member

@markborkum Any last thoughts? I'm good.

@markborkum
Copy link
Contributor Author

@dmlb2000 Ship it!

Copy link
Member

@dmlb2000 dmlb2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me

@dmlb2000 dmlb2000 merged commit a1938e2 into pacifica:master Sep 19, 2018
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.

2 participants