Updating choices for single language. #376
Conversation
|
The approach here is to look at the first choice in a list and see whether its label is represented as a dict or a simple value, right? This seems fine to me! I'm guessing you looked at doing it with the header row of the choices sheet and that there's no way to do it with that? I think the conditionals at https://github.com/XLSForm/pyxform/pull/376/files#diff-642ca628a5111f8d9ff1314acb61ceadL550 would also go away. |
|
Yes, this is looking at the label whether it is dictionary type or simple value. |
|
I can do checking on the row header and that's the other approach that I mentioned in the ticket. What that entails:
What do you think? |
|
I don't see a big benefit to having the property exposed in the json_dict. The checks are cheap and I think they're pretty easy to understand. I see also that on the issue you asked about modifying the expected XML. I don't think there's going to be any way around that. Hopefully there aren't too many tests affected and the diff will be easy to review. |
|
Kewl then @lognaturel. Going to fix the expected xml next then. There are 5 xml files I think. Not bad. |
Codecov Report
@@ Coverage Diff @@
## master #376 +/- ##
==========================================
+ Coverage 82.33% 82.48% +0.14%
==========================================
Files 23 23
Lines 3244 3260 +16
Branches 757 763 +6
==========================================
+ Hits 2671 2689 +18
+ Misses 436 431 -5
- Partials 137 140 +3
Continue to review full report at Codecov.
|
|
Unit tests fixed and I added a few tests. Ready for some eyes to check it out. |
|
|
||
| def test_inline_translations(self): | ||
| """ | ||
| Test using data as the name of the form which will generate <data />. |
There was a problem hiding this comment.
This comment isn't relevant
There was a problem hiding this comment.
Updated the comment. It was a copy-pasta error.
|
|
||
| def test_multiple_translations(self): | ||
| """ | ||
| Test using data as the name of the form which will generate <data />. |
There was a problem hiding this comment.
Comment should relate to the test. This one is actually a little odd because the reason that translations are generated is because the survey sheet only has a default language (label without a suffix) and the choices sheet only has English (en). I think that's ok but to be more explicit, consider adding a language suffix.
What happens if both have the ::English(en) suffix? No translations are generated, right? Or is it only the case if the settings sheet explicitly sets English(en) as the default? Since this case is a little unclear, I think it's worth having a test for.
There was a problem hiding this comment.
I think since I'm checking it whether it's a dictionary or a straight string, whenever there's ::English(en), it will think the survey is multi-language because it will actually generate a dictionary instead of the regular label.
Do you want me to inline this as well? This will require an update in the approach to test for multi-language or not. Instead of checking the first element to see if it's a dictionary or not, i need to count each element in the choices dictionary. Multi language will have more than 1 elements in the choices.
|
It looks like the tests fail on some Python version and that black fails on others. Would be good to look into. |
|
Fixed for 3.5. I ended up need to sort the label and the name to make sure they're consistent on both 2.7 and 3.5. I tried to fix the black issue earlier, but failed miserably hahaha ... |
ukanga
dismissed
their stale review
@nribeka, can you fix the conflict first? Otherwise, this looks good.
Conflicts: pyxform/tests/test_output/cascades_old.xml
Fix #285.
This will inline the label when there's only 1 language for the survey instead of referencing the itext fields.