791: fix support for a source referencing an ancestor repeat target #794
Conversation
- unlikely to be a problem but if iter_ancestors yields nothing then the default for `next()` should be an iterable so it can be unpacked like the expected values are.
- makes debugging quite a bit more convenient
- previous test cases focussed on the references targeting questions but as noted in test_references__source_under_target it's useful for a source to be able to refer to its parent repeat - the test build_survey_from_path_spec syntax is modified to recognise "at" as the target which is also an ancestor repeat of the source. - SurveyElement.lowest_common_ancestor is changed to only deal with finding common ancestors, rather than identifying any other node relationship types (e.g. parent / child / ancestor).
|
The CI test failure here is due to the issue described in this comment. |
lognaturel
left a comment
lognaturel
left a comment
There was a problem hiding this comment.
Thank you! Looks good to me but some of the tests would benefit from referencing real nodes, I think. Their assertions are technically correct but feel more confusing than necessary.
I also want to note that there is ambiguity in the targeted expressions. Someone could conceivably use a ${} from within a repeat and want a relative reference. But I think the behavior from this PR is more likely to match their expectations. I assume that if someone knows how to formulate this kind of more complex expression they can also write relative references themselves comfortably. Most importantly, we didn't intend to change the behavior from what it was before.
- the tests passed before since they are about resolving `t` relative to `s` but for completeness/accuracy the `default` path now matches the actual relative position of `s`.
|
Thanks I updated the test case paths to match the form structure in 5b0289a. Not sure if it's what you meant but the tests added in this PR under TestGetPathRelativeToLCAR show that if the target repeat (parent of source) is itself inside a repeat then the path will be relative (e.g. |
lognaturel
left a comment
lognaturel
left a comment
There was a problem hiding this comment.
Thanks! It feels easier to understand when the paths exist. Of course the expressions with repeats still wouldn’t really make sense because they return nodesets but at least the shape of the paths look right.
There are a couple things in the other PR I’m not yet understanding. I’ll finish that review after some rest! I think it will be fine to merge first.
2 participants
Closes #791
Why is this the best possible solution? Were any other approaches considered?
See commit messages for details.
Considered keeping lowest_common_ancestor behaviour of identifying other node relationship types but the name of the function is about common ancestors and it's only used for that purpose.
What are the regression risks?
Fixes a regression.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No
Before submitting this PR, please make sure you have:
testspython -m unittestand verified all tests passruff format pyxform testsandruff check pyxform teststo lint code