lognaturel commented 3 years ago

Closes #515

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

This is a simplification of #518. I aimed to have a single function that identifies the dynamic label case and to make sure that the checks on conditions for using itext are easily readable. I considered seeing if we could avoid the redundancy between those checks but that felt too involved for the moment.

What are the regression risks?

I don't think there's a risk of degraded user experience. I think the worst case would be that we might be generating itext too often now but that would generally be safe.

Another risk is related to behavior I changed for consistency that affects selects from itemsets with media. Previously, both itext and label children were generated in the secondary instance. This is unnecessary but I can imagine some downstream analysis tools or servers relying on it. I think it's clearly better to omit so I think it's ok.

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

It would be good to explicitly mention that we can use references for labels but not for names or other columns. I've filed https://github.com/getodk/docs/issues/1329. I'm not sure it fits in to xlsform.org.

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

codecov-io commented 3 years ago

Codecov Report

Merging #521 (cdcb6ba) into master (bee9654) will increase coverage by 0.03%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #521      +/-   ##
==========================================
+ Coverage   83.92%   83.95%   +0.03%     
==========================================
  Files          25       25              
  Lines        3713     3721       +8     
  Branches      865      868       +3     
==========================================
+ Hits         3116     3124       +8     
  Misses        452      452              
  Partials      145      145

Impacted Files	Coverage Δ
pyxform/question.py	`93.42% <100.00%> (+0.05%)`	:arrow_up:
pyxform/survey.py	`92.51% <100.00%> (ø)`
pyxform/utils.py	`84.86% <100.00%> (+0.62%)`	: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...5362a8f. Read the comment docs.

gushil commented 3 years ago

@lognaturel @MartijnR Should we merge this instead of PR #518 ?

MartijnR commented 3 years ago

Up to @lognaturel, but this looks like a cleaner solution. Thanks @lognaturel!

lognaturel commented 3 years ago

Yes, please do a final review on this one, @gushil and @MartijnR.