Skip to content

Fix parsing of end and step in array slice#37

Merged
dmlb2000 merged 7 commits intopacifica:masterfrom
markborkum:bugfix/parse-arrayslice
Jan 7, 2020
Merged

Fix parsing of end and step in array slice#37
dmlb2000 merged 7 commits intopacifica:masterfrom
markborkum:bugfix/parse-arrayslice

Conversation

@markborkum
Copy link
Copy Markdown
Contributor

Description

This issue is caused by the way that ANTLR4 indexes terminals in a context. In ANTLR4, when two or more terminals with the same name are present in the same production, but only a subset of terminals are parsed, the indexing does not reflect the position of the terminal within the production.

For #35, when the string "::n" is parsed, for a given "n", the context contains only 1 "NUMBER" terminal, which is assigned the index 0, where the index 1 would be expected (with the index 0 returning None or a similar sentinel value).

The mitigation is to test for the presence of the second "COLON" terminal and then to verify that it came before the second "NUMBER" terminal.

Issues Resolved

Fixes #35

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 December 20, 2019 18:19
@dmlb2000
Copy link
Copy Markdown
Member

dmlb2000 commented Dec 30, 2019

The no cover pragma gives me pause. We do have a lot of array slice tests

class TestArraySliceSubscript(TestCase):

Is there some antlr magic that means we don't get coverage over the missing lines?

The tests are for the array slice subscript method mostly not the upper level parser itself.

@markborkum
Copy link
Copy Markdown
Contributor Author

arrayslice_subscript_test.py does not invoke the parser.

There are some array slice tests in bookstore_test.py that invoke the parser, but I guess that there is not full coverage of all possible combinations of start/stop/step.

dmlb2000 added a commit to dmlb2000/python-jsonpath2 that referenced this pull request Dec 31, 2019
This adds two tests to complete the coverage nessisary for
pacifica#37

Signed-off-by: David Brown <dmlb2000@gmail.com>
This adds two tests to complete the coverage nessisary for
pacifica#37

Signed-off-by: David Brown <dmlb2000@gmail.com>
The grammer did not allow parsing of sliceable expressions with a
trailing colon. In Python at least, slices like `[:2:]` and `[2::]`
are allowed.

This change makes the sliceable expression in the grammer allow
the last number to be optional.

Signed-off-by: David Brown <dmlb2000@gmail.com>
@dmlb2000
Copy link
Copy Markdown
Member

dmlb2000 commented Dec 31, 2019

@markborkum I created a pull request to your branch to complete the coverage testing without ignoring the code. I also found a situation that doesn't parse correctly.

Please checkout markborkum#1 then we can merge this pull request.

I did check coverage and adding the additional testing does cover the method without the no cover pragma.

Add extra tests to complete coverage.
Copy link
Copy Markdown
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.

I'm fine with this pull request.

@dmlb2000 dmlb2000 merged commit 882af98 into pacifica:master Jan 7, 2020
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.

Results do not match other implementations

2 participants