-
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 #467
Conversation
Codecov Report
@@ Coverage Diff @@
## master #467 +/- ##
==========================================
+ Coverage 83.39% 83.53% +0.14%
==========================================
Files 25 25
Lines 3601 3633 +32
Branches 836 842 +6
==========================================
+ Hits 3003 3035 +32
Misses 454 454
Partials 144 144
Continue to review full report at Codecov.
|
b09cb7c
to
b4b0683
Compare
782eb1e
to
361834c
Compare
361834c
to
f79ade9
Compare
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.
Thanks! It would be really helpful to get a high-level view of the decisions you made and some risk analysis. There are clearly ramifications if downstream tools use the itext ids in some way. I don't know how likely that is. Did you think through that?
If we're going to be changing the structure of the itext ids, we should consider alternatives. For example, we could use uuids for those identifiers instead of using the list and choice names. I don't really understand the static_instance
prefix. If it's just noise, could we remove it?
@@ -75,3 +75,41 @@ def test_missing_media_itext(self): | |||
], | |||
itext__excludes=['<value form="audio">jr://audio/-</value>'], | |||
) | |||
|
|||
def test_select_with_choice_filter_translation(self): |
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.
This test would pass on master
, right? You want to make sure to have a test that would have failed so that it tests the new code paths. I think you can do that by removing the label::Latin
column. I highly recommend using a test-driven approach in this code base -- typically it's possible to add tests without much refactor which is great! That way you can really go through the red-green-refactor cycle.
I would expect at the very least test_select_with_media_and_choice_filter_and_no_translations_generates_media
and test_select_with_choice_filter_and_translations_generates_single_translation
or something like that to capture the desired behavior for the two issues you're closing.
if multi_language: | ||
itext_id = "-".join(["static_instance", list_name, str(idx)]) | ||
if multi_language or has_media: | ||
name = choice.get("name").replace(" ", "_") |
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.
Why add this? Wouldn't a name with a space fail earlier because it's not a valid XML identifier? If it's possible to have a name with a space in it, please add a test to cover that case.
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.
It seemed to be possible for this test, I'll need to confirm this though. I'll confirm and add a test if this is the case.
itext_id = "-".join(["static_instance", list_name, str(idx)]) | ||
if multi_language or has_media: | ||
name = choice.get("name").replace(" ", "_") | ||
itext_id = "-".join(["static_instance", list_name, name]) |
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.
Is this change from index to name so that all identifiers have a consistent structure? Why is this important? Do you know what the static_instance
prefix is supposed to mean (I don't). What are the implications of changing this for all translations?
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.
It's to make the ID construction more predictable. Noticed that the itext entry creation happens in multiple areas
Lines 594 to 610 in f79ade9
for element in self.iter_descendants(): | |
for d in element.get_translations(self.default_language): | |
translation_path = d["path"] | |
form = "long" | |
if "guidance_hint" in d["path"]: | |
translation_path = d["path"].replace("guidance_hint", "hint") | |
form = "guidance" | |
self._translations[d["lang"]][translation_path] = self._translations[ | |
d["lang"] | |
].get(translation_path, {}) | |
self._translations[d["lang"]][translation_path].update( | |
{form: {"text": d["text"], "output_context": d["output_context"],}} | |
) |
Lines 613 to 629 in f79ade9
for list_name, choice_list in self.choices.items(): | |
multi_language = isinstance(choice_list[0].get("label"), dict) | |
has_media = bool(choice_list[0].get("media")) | |
if not multi_language and not has_media: | |
continue | |
for choice in choice_list: | |
for name, choice_value in choice.items(): | |
name = choice.get("name").replace(" ", "_") | |
itext_id = "-".join(["static_instance", list_name, name]) | |
if isinstance(choice_value, dict): | |
_setup_choice_translations(name, choice_value, itext_id) | |
elif name == "label": | |
self._add_to_nested_dict( | |
self._translations, | |
[self.default_language, itext_id, "long"], | |
choice_value, | |
) |
Line 651 in f79ade9
def _setup_media(self): |
Not entirely sure what the static_instance
prefix means... Changing this for all translations related to choices(with multiple languages / media) would mean that all itext entries will have the static_instance-<list_name>-<name>
format which seemed a bit more consistent and predictable, I don't believe this would affect any other functionality
@@ -546,6 +547,15 @@ def _set_translations(self): | |||
assert "text" in self.translations[0] | |||
assert "lang" in self.translations[0] | |||
|
|||
def _get_list_name(self, itemset): |
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.
This doesn't look right. You should be able to get that instance name from the instance declaration.
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.
Mhm I might have been looking at the wrong area, I might have to look at the functionality that converts the XML to a dict. By this point the list_name
info doesn't seem to have been retrieved... Seems to just be a breakdown of the various sections of the XLSForm, didn't contain the list_name
within it's contents.
Closes #464, #463
Why is this the best possible solution? Were any other approaches considered?
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.
No
Before submitting this PR, please make sure you have:
tests_v1
nosetests
and verified all tests passblack pyxform
to format code