#653 Detection of multilanguage choices only works if first choice list is multilanguage #666
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lindsay-stevens
force-pushed
the
pyxform-653
branch
3 times, most recently
from
391f161
to
3a008d1
Compare
- overall goal is to avoid using only the first few lines of survey
or choices sheet data to decide whether to use multi-language
behaviour (itext) or not.
- xls2json.py: re-use dealias func for finding headers as well as data
- translation_checks.py: use headers data to find translations
- survey.py:
- declare _translations key and store and translations info in it
- tidy setup_translations to data prep vs. _translations update steps
- add translation type key to help differentiate question vs choice
- annotate survey elements with info about translations, dynamic
choices, and media, so that this doesn't need to be re-checked later
by individual elements.
- decide whether to emit itext labels for choices on a per-itemset
basis, rather than switching into itext mode for all choices. This
avoids needing to mangle untranslated itemsets into translated when
other itemsets in the survey are translated (otherwise they would
get no itext, not even a hypen "-").
- add lru_cache upper limit so that it doesn't grow forever
- update add_to_nested_dict to allow updating existing dict
- question.py:
- declare translations, dynamic choices, and media keys set by survey
and use them to determine relevant behaviour instead of re-checking
- utils.py: remove multi-language parameter from has_dynamic_label
because it's clearer to do the same logic external to the function.
- add tests for multi-language detection + with media
- For insert/pop on the left side: O(1) for deque vs. O(n) for list. - Avoids creating lots of new lists with [] + []. - Did not investigate whether the "flat" filter is still necessary.
- needed after rebase on latest master
lindsay-stevens
force-pushed
the
pyxform-653
branch
from
3a008d1
to
05ad654
Compare
lognaturel
approved these changes
Dec 1, 2023
There was a problem hiding this comment.
it could have looked across all itemsets but then untranslated itemsets would've had to be mangled into translations
This is very interesting! We warn about this case and it's probably not terribly useful so either way would be fine.
Here's an interesting form I cooked up: https://docs.google.com/spreadsheets/d/1NW2yfyLCavbJ8reUo-6i1yjq48eIaEJ170ggDEzYB8Y/edit#gid=1033515766
I'm happy with the output and you may find it interesting to look at.
|
There's a small recommendation and a question about risk inline but I'm going to merge now so we can get it onto staging asap. |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #653
Why is this the best possible solution? Were any other approaches considered?
There are 2 main problem areas addressed: the translation warning, and the detection of translations for triggering multi-language (itext) output behaviour,
This changes the missing translations check in xls2json to use column headers instead of the first row. To avoid re-iterating the sheet data, the existing dealias function is modified to track and return the headers as well as the sheet data.
This changes multi-language detection by doing so only in the Survey object. For the Question sub-types, Survey sets the relevant info with new fields, so that the Questions don't need to re-check. For choices, the detection is per itemset (it could have looked across all itemsets but then untranslated itemsets would've had to be mangled into translations).
What are the regression risks?
Since there are some new fields on the Survey and Question classes, external library users that pass in JSON without these may experience errors or inconsistent behaviour. There is a file
json_form_schema.jsonbut it seems to be not consistently updated, and I don't know if it's accurate - there doesn't seem to be a pyxform way to generate it or use it to validate.Does this change require updates to documentation? If so, please file an issue here and include the link below.
Probably not, it's all internal representation.
Before submitting this PR, please make sure you have:
testsnosetestsand verified all tests passblack pyxform teststo format code