-
Notifications
You must be signed in to change notification settings - Fork 136
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixed duplicate label translations for secondary itemsets and use itextID for selects with choices that have media specified #468
Conversation
… label translation
…ter and translations
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
The prefix does not seem to serve any noticeable purpose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
285ae96
to
8947a37
Compare
8947a37
to
3ffb1ac
Compare
I'm glad. I've gone through, commented, and made changes. Regarding the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now feel I can fully explain this. It makes a lot of sense and seems low-risk. Thanks, @DavisRayM! It's great to see some redundancy go away.
Sorry if you saw a weird comment about a remaining issue come through -- I wasn't on the latest commit.
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 elementsWhat 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:
tests_v1
nosetests
and verified all tests passblack pyxform
to format code