-
Notifications
You must be signed in to change notification settings - Fork 7
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
xpath: [incorrect] support for indexed-repeat
function
#150
Closed
Closed
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
9aad48b
chore: xpath package consistent test suffixes…
eyelidlessness 9fdf3e2
Fix (scenario): create missing repeat instances on `answer` call…
eyelidlessness 56df0ea
xpath: add support for `indexed-repeat` function
eyelidlessness 66aa8c9
scenario: update tests for `indexed-repeat` support
eyelidlessness a781d92
Changeset: `indexed-repeat`
eyelidlessness File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 algorithm here is something like find the last positional predicate, strip it off, use that reference to find the repeat range, add instances up to the position accessed, right? So it won't work for nested repeats that all have missing instances (e.g. /data/repeat1[2]/repeat2[5]/foo)? I think maybe there are tests that use that in JR but I'm not entirely sure. Fine to leave as-is for now but wanted to check my understanding.
I don't think this requires immediate actions either but I wanted to mention that I expected some slightly higher-level building blocks here! Specifically, I'm surprised to see some regex for getting references without predicates and for getting sub references. I imagine at some point it will be worth having some reusable support for those concepts.
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 comment about building blocks is partially addressed at 66aa8c9#diff-f3ac6c9869393022cd23375d383ded81f1d35c2c962125846cb288792ee28d52R15
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've been mulling this since I saw these comments yesterday, and my instinct is that we're probably better off not porting this aspect of
Scenario
.The tests I've encountered which assume this behavior have frequently surprised and confused me. And not in a trivial way: almost every time, I've misinterpreted the nature of a failure, or misunderstood the intent of the test, sending me off on unproductive tangents.
Tests which expect this level of logical complexity should probably have their own logic tested as well. For instance, I was already deeply uncomfortable with the level of complexity in
child-vaccination
. There is significant application-like work happening there, and I find it vastly harder to understand the test flow than any of the tests which more explicitly express what they are doing.I should say that I regret the linked comment. In hindsight, the kind of expression analysis and manipulation that I discuss there, and that you say you were expecting here, is definitely appropriate for engine implementation details. I am quite cautious about using logic of that kind to implement test behavior.
On Tuesday we discussed the fact that such logic is in fact incoming, supporting a variety of other engine improvements. And as we discussed then, that logic is downstream from this change, as other affected tests would currently depend on this specific implicit repeat instance creation. But to be clear, as that work took form, it felt pretty clear that the expression analysis/manipulation logic will be entirely encapsulated in the engine. If we do anticipate exposing arbitrarily/increasingly complex expression analysis and manipulation as a public API, I think we should really think hard about what the appropriate design for that would be. And the appropriate package and/or abstraction layer from which we should expose it.
I already wanted to make repeat instance creation more explicit before this discussion, but I hesitated in this PR on the basis that it would increase divergence from JavaRosa. But that divergence is much more appealing to me now, in light of some of the implications this has raised.
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 thought your linked comment meant in the engine. Agreed it’s the engine that eventually should have that functionality so good to see it’s coming. 👍
As for not porting the implicit repeat addition, then I think we should probably change the JR tests. I don’t know off the top of my head how many are affected and will look.