XLSForm / pyxform

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

Fixed duplicate label translations for secondary itemsets and use itextID for selects with choices that have media specified #468

Closed DavisRayM closed 3 years ago

DavisRayM commented 3 years ago

Based on #467 Closes #464, #463

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

The simplest approach(Least amount of modification to already existing functionality). Ensures that we only generate translations for choices in filtered selects in a controlled manner and that itext is used for selects with media

Considered utilizing uuids, but decided otherwise since uuid generation is unpredictable; _Translation entries are created/linked in multiple areas i.e _setup_media & _setup_translations using uuids may lead to more duplicate entries and I'm not entirely sure of an easy way to implement uuids and correctly link them to the question elements_

What are the regression risks?

None

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:

codecov-commenter commented 3 years ago

Codecov Report

Merging #468 into master will increase coverage by 0.16%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #468      +/-   ##
==========================================
+ Coverage   83.39%   83.55%   +0.16%     
==========================================
  Files          25       25              
  Lines        3601     3612      +11     
  Branches      836      839       +3     
==========================================
+ Hits         3003     3018      +15     
+ Misses        454      452       -2     
+ Partials      144      142       -2     
Impacted Files Coverage Δ
pyxform/question.py 92.95% <100.00%> (+0.10%) :arrow_up:
pyxform/survey.py 92.54% <100.00%> (+1.02%) :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 50921a8...47072dc. Read the comment docs.

DavisRayM commented 3 years ago

Really liking those tests, @DavisRayM, thanks for the simplified PR! My review process is to look at the tests first, then go through the code and if I can't explain a section, comment it out and run the test suite to see what it does. I've identified two code sections that I don't understand and can currently be commented out without any tests failing. Please illustrate the cases they cover with tests if they are indeed needed.

I like the removal of the static_instance prefix but it seems possibly risky so I'd like another reviewer's thoughts on it.

One difference between your prior PR and this one is that the other one updates xform2json to read in secondary itemset translations. I generally consider xform2json experimental and unsupported so I don't think it's necessary to update. I just wanted to flag it in case that was something you intended to do.

I'm glad. I've gone through, commented, and made changes. Regarding the xform2json changes, I think I'll need to research a bit more into the functionality. Currently, with these changes, the secondary itemset translations don't seem to be a problem(the JSON generated version of the specify_other.xls fixture seems to be passing the test). I'll look into it though and open an issue if I spot an issue with the secondary itemset translation on that functionality.