${} references in predicates should be expanded using current() #524
Conversation
Thanks. I will add those tests. |
Codecov Report
@@ Coverage Diff @@
## master #524 +/- ##
==========================================
+ Coverage 83.92% 83.93% +0.01%
==========================================
Files 25 25
Lines 3713 3716 +3
Branches 865 866 +1
==========================================
+ Hits 3116 3119 +3
Misses 452 452
Partials 145 145
Continue to review full report at Codecov.
|
|
Addressed your feedbacks in recently pushed commits. Thanks. |
pyxform/survey.py
Outdated
| # (starts with 'instance(' and have square bracket in it) | ||
| is_secondary_instance = ( | ||
| matchobj.string.startswith("instance(") | ||
| and re.search(r"\[(.+)\]", matchobj.string) is not None |
There was a problem hiding this comment.
Sorry for my confusing comment. I think the square bracket (i.e. predicate) check may not be necessary (like you had initiallly), since the use of ${node} syntax implies this. But would be good to hear what @lognaturel thinks.
There was a problem hiding this comment.
Apologies for the delay on this!
I agree with @MartijnR that looking ONLY for instance( as you had before is a clever way to identify expressions that will need current. As far as I can tell, the only place there could be a ${} reference in such an expression is in a predicate.
The issue with this is that it will only work if a complete expression starts with instance(. Consider for example changing test_repeat_using_select_with_reference_path_in_predicate_uses_current to have ${item-counter} + instance('item')/root/item[itemindex=${item-counter}]/label (it's nonsense but illustrates the point). Now current() isn't used anywhere.
Thanks for the review @lognaturel Commit ae2125e includes fixes for this. |
|
@lognaturel - Is there anything @gushil or I can do to help with this issue? |
@pbowen-oc Y'all have cycles for some of my other work? ;) Taking a look... |
There was a problem hiding this comment.
Thank you, this approach makes sense to me semantically -- check to see if a ${} expression is in a predicate for a path expression that starts with instance(. This will slow form conversion down since every single ${} expression will go through an extra check but I can't think of a great alternative.
I can't think of additional test cases. I would like the naming in the code changes to be a little more precise.
pyxform/survey.py
Outdated
| instance_regex = re.compile(r"instance\([^)]+\S+") | ||
|
|
||
| def _is_secondary_instance(): | ||
| """check if current matchobj is a predicate and part of primary instance""" |
There was a problem hiding this comment.
I think we're really going to tie ourselves up in knots if this code needs to be revisited so making sure the naming is precise is important.
Name: _in_secondary_instance_predicate
Comment: check if ${} expression represented by matchobj is in a predicate for a path expression for a secondary instance
@MartijnR any other ideas for tightening the wording?
There was a problem hiding this comment.
Thanks for the suggestion @lognaturel
Fixed in the next commit.
Closes #490
Why is this the best possible solution? Were any other approaches considered?
I use simple predicate testing by checking whether current context start with
instance(. I'm thinking about using external library for xpath checking, but thought that would be too overkill for this case.What are the regression risks?
None, but probably still need some more tests. Open to more tests suggestion!
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:
tests_v1nosetestsand verified all tests passblack pyxformto format code