${} references in predicates should be expanded using current()

[gushil]

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:

• [x] included test cases for core behavior and edge cases in tests_v1
• [x] run nosetests and verified all tests pass
• [x] run black pyxform to format code
• [x] verified that any code or assets from external sources are properly credited in comments

[gushil]

I added some tests to the issue, that may be helpful if this code ever gets refactored.

Thanks. I will add those tests.

[codecov-io]

Codecov Report

Merging #524 (f176380) into master (bee9654) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            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              

Impacted Files	Coverage Δ
pyxform/survey.py	92.56% <100.00%> (+0.04%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update bee9654...f176380. Read the comment docs.

[gushil]

@MartijnR

Addressed your feedbacks in recently pushed commits.

Thanks.

[gushil]

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 https://github.com/XLSForm/pyxform/pull/524/commits/ae2125e67a1bd92fe7ebbba46c89f9c7d7d33748 includes fixes for this.

[pbowen-oc]

@lognaturel - Is there anything @gushil or I can do to help with this issue?

[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...