XLSForm / pyxform

A Python package to create XForms for ODK Collect.
BSD 2-Clause "Simplified" License
77 stars 134 forks source link

Generate correct refrence when selecting choices from current repeat question #516

Closed DavisRayM closed 3 years ago

DavisRayM commented 3 years ago

Closes #503

Why is this the best possible solution? Were any other approaches considered?

Simplest solution with minimal changes

What are the regression risks?

N/A

Does this change require updates to documentation? If so, please file an issue here and include the link below.

None

Before submitting this PR, please make sure you have:

lognaturel commented 3 years ago

From Slack: "Especially this change here. I'm unsure of whether the path change is referencing something different... Would a change like that mean that it won't pick the value from the current repeat ?"

Is that what makes it a WIP or is there anything else that you expect to do here, @DavisRayM?

DavisRayM commented 3 years ago

From Slack:

"Especially this change here. I'm unsure of whether the path change is referencing something different... Would a change like that mean that it won't pick the value from the current repeat ?"

Is that what makes it a WIP or is there anything else that you expect to do here, @DavisRayM?

Yes, that's the only bit of that I wasn't too sure about

lognaturel commented 3 years ago

The general case is "if you need to reference a repeat nodeset, your expression must include the repeat node explicitly". In the case of itemsets from repeats, there's always a predicate expression after a nodeset expression. There are cases like parameters to the count function where we need to access the raw nodeset. The problem with a relative expression such as ../[predicate] in the form in the issue is that ../ references a single repeat instance, not the whole nodeset.

In the case of ${target_min_age}, we're not referencing the nodeset, we're referencing a specific value in a specific repeat instance.

codecov-io commented 3 years ago

Codecov Report

Merging #516 (fa1afbd) into master (43ea039) will decrease coverage by 0.09%. The diff coverage is 100.00%.

:exclamation: Current head fa1afbd differs from pull request most recent head a2f784b. Consider uploading reports for the commit a2f784b to get more accurate results Impacted file tree graph

@@            Coverage Diff             @@
##           master     #516      +/-   ##
==========================================
- Coverage   83.94%   83.85%   -0.10%     
==========================================
  Files          25       25              
  Lines        3719     3697      -22     
  Branches      867      862       -5     
==========================================
- Hits         3122     3100      -22     
  Misses        452      452              
  Partials      145      145              
Impacted Files Coverage Δ
pyxform/question.py 93.36% <100.00%> (-0.06%) :arrow_down:
pyxform/survey.py 92.49% <100.00%> (-0.06%) :arrow_down:
pyxform/utils.py 84.24% <0.00%> (-0.63%) :arrow_down:
pyxform/xls2json.py 78.33% <0.00%> (-0.20%) :arrow_down:
pyxform/constants.py 100.00% <0.00%> (ø)

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 43ea039...a2f784b. Read the comment docs.

DavisRayM commented 3 years ago

@lognaturel Kindly re-review when you get a chance