603: Always generate secondary instance for selects #614
Conversation
lindsay-stevens
force-pushed
the
pyxform-603
branch
from
4dd99fb
to
3248805
Compare
- survey.py: previous code would output position-based and reference-based translations (e.g. mylist-0 and choice:label), so it now skips reference-based translations for select_types. - survey_element.py: related to the above change, test_xform2json.py test_convert_toJSON_multi_language failed due to circular reference. Change traverses translations to remove offending objects. - pyxform_test_case.py: added the test case number to the debug/fail output, to help when there are many XPath expressions. - numerous test fixes to accommodate updated choice output.
- now covered by tests/xpath_helpers/choices.py
lindsay-stevens
force-pushed
the
pyxform-603
branch
from
783f1f1
to
ab992b1
Compare
- The test was broken by changes to choices processing (XLSForm#603). - The test conditions seem to relate only to the instance_xmlns feature. - The new test asserts the same using the pyxformtestcase pattern.
- OK locally but failing on CI
- builder.py: add type hint, remove unnecessary dict
- xform2json.py:
- _set_survey_name now handles multiple instances
- _set_submission_info now sets itemset details from secondary
instance, and attempts to parse choice filters
- _set_translations now returns value to be set in __init__()
- _get_text_from_translation now does media comparison as a list,
since a dictkeysview is not a list
- add _get_choices to include choices in internal xform representation
- xls2json.py
- skip adding choice lists that aren't used in the xform to avoid
extra unnecessary data. Maybe worth a warning or maybe not.
- add secondary instance for table-list appearance, but reject it if
it was being added with a choice filter (not supported).
- xform2json_test.py:
- expand test_load_from_dump variables for easier debugging
- add test cases for choice filters, field-list and table-list
- these test cases can be refactored to PyxformTestCase at some stage
- xls2xform_tests.py: use equivalent test method instead of keyword
- update expected XML from inline choices to secondary itemset for the
following (these can be refactored to XPath PyxformTestCase):
- test_table_list.py
- repeat_date_test.xml
- xml_escaping.xml
- reasonably confident that these are the expected types
- section should import SurveyElement from where it is defined. - A NotImplementedError is commonly used to indicate a base method that should be implemented in child classes, and it is used in SurveyElement for xml_control(). The Option and Tag classes don't seem to need this method, so adding an implementation that raises this error is a bit clearer.
lindsay-stevens
force-pushed
the
pyxform-603
branch
from
8f72e42
to
7015ba7
Compare
- move choices logic off to new function add_choices_info_to_question()
- exclude select with appearance=search() from itemset (per comment)
- new func for readability and to clarify what info is used to
determine choices structures
- survey.py: skip adding translations for choices, instead of just for
choice_filter. Use type "Option" to detect choice elements which
should be more reliable. Move get_xpath lookup under if condition so
it's only called if needed.
- xls2json.py: fix lookup of select type "select one external" which
has an alias "select_one_external". Docs use the underscore alias
but the implementation is looking for the spaces alias.
- xlsform_test_case/base.py: simplify reporter function
- xlsform_spec_test.py: simplify repetitive tests by adding test func.
Ideally these should be re-implemented as PyxformTestCase, but they
cover a lot of scenarios so for now just simplifying the structure.
- test_expected_output/*.xml: update expected XML to match current
behaviour, generally: 1) update body to refer to instance, 2) add new
instances in model, 3) add new translations in itext, 4) remove old
itext from translations.
- Misc minor fixes:
- utils.py: fix deprecation warning for get_sheet_by_name.
- replace in-line regex with pre-compiled regex, and replace match
(only looks at start of string) with search (looks at whole string).
- add more constants for commonly-used internal strings so that it's
easier to find where they are used, added usages to xls2json.py.
- The right number of empty rows would be removed afterwards anyway, but for someone reading the code later it'd be better if that number was as stated.
- yes_or_no_question and xml_tests updated for new itemset output for selects. The default_survey_sheet test expected no default_form_name. - add type hints to workbook_to_json and parse_file_to_json.
lindsay-stevens
force-pushed
the
pyxform-603
branch
from
7015ba7
to
2adb257
Compare
There are three reasons we've discussed:
Of those, I think the second point has the highest risk. We've had a post on the forum announcing this change for a year, though, so hopefully folks building downstream tools have had time to adapt. Have you looked at |
|
sorry, I only just noticed this the other day (I get spammed by so many git updates I tend to tune them out... :-( Would it be possible to add a commandline option to continue to generate 'inline' select option when desired? I do actually currently rely on them because I havent completed parsing secondary instances in my (iOS) XForms parser yet. And given it is such a breaking change, it might still be nice to still support it (as deprecated), even if you have you now explicitly enable it via said option. Otherwise I guess I'll have to fork the current version and use that in the interim. |
|
@lognaturel there's a few more translations test to update, then the last implementation thing to look at should be |
|
More conversation with @tiritea on the forum.
Really. And not an itemset AND static choices, only an itemset? I could have sworn I checked that as part of my comment long ago. Well, that makes that easy at least.
Yes, that's right. |
|
@lognaturel I meant:
|
|
Got it, thanks! |
|
FWIW I dont support search() yet, either, so please dont do anything funky here on my account... (I'll deal with all that after I get secondary instances working). |
- test_translations.py: - remove xpath helpers now in xpath_helpers/choices.py - update test xpath assertions to match itemset behaviour - choices.py: - improve xpath helpers to not require specification of position
- builder.py:
- add or_other choice to survey-level choice dict
- add type annotations
- constants.py: add new constant for "or specify other" internal suffix
- xls2json.py: add error for choice_filter + or_other. Main reason I
suppose is that it's unknown how to filter the "other" choice so the
user might never see it.
- xlsform_spec_test.py:
- fix or_other expected XML to include "other" and match sorted
element names.
- move or_other test into this testcase since it is the same as others
there already.
- builder_tests.py: update expected output to match or_other output.
lindsay-stevens
force-pushed
the
pyxform-603
branch
from
3275b6c
to
751e4ea
Compare
|
@lognaturel ready for review. It's covering the cases mentioned in enketo/enketo#216, and any tests that failed as a result of code changes. I haven't combed through every test case though, e.g. in case there's some way to avoid generating itemsets. |
|
Finally spending some focused time on this after speaking to @lindsay-stevens about it at the ODK Summit. We would like to release this by mid-August at the latest. My review checklist:
Haven't looked into what's going on yet. Ideally pyxform output could be modified slightly to not get those differences but it might not be possible.
|
There was a problem hiding this comment.
This looks really thoughtfully done. There's just one section I really can't wrap my brain around and makes me nervous. I've marked it inline.
There's some Enketo rendering weirdness that I think we should look into. Maybe it's just the spacing in the pyxform output? See above.
I still want to look a little bit more at translations and long/short forms but overall I think this is very close to merge.
| @@ -244,6 +244,13 @@ def to_json_dict(self): | |||
| result["children"] = [] | |||
| for child in children: | |||
| result["children"].append(child.to_json_dict()) | |||
| # Translation items with "output_context" have circular references. | |||
There was a problem hiding this comment.
output_context is a way to save the node reference for cases where there are output nodes so that ${} references can be expanded? I tried removing this scary bit of code and test_convert_toJSON_multi_language did fail. Then I set all places where output_context is set to None and some tests in test_repeat failed. I don't understand why this works, though. I thought it would remove all output_context values for all survey elements.
There was a problem hiding this comment.
This code is reached by a call to survey.to_json so it shouldn't affect form conversion to XML.
The purpose of output_context is to hold a reference to a SurveyElement, so that references like ${} can be expanded. In this case we're going from "internal PyForm" structures into JSON - so I don't know what the intended JSON representation of output_context should be (maybe a copy of the node dict?). Or maybe the JSON output should have the de-referenced output somewhere (e.g. a string containing XML). It seems like it needs a separate issue to design and implement, or document.
Anyway, the problem at hand is that the node reference is a SurveyElement, which has a reference to it's parent Survey, which has references to it's child SurveyElements, which have references to the parent Survey, and so on. To break out of that loop, output_context is deleted so that survey.to_json() can return a document - it just would have the original referencing strings like My fav fruit is ${fruit}, and de-referencing those is left to the caller.
There was a problem hiding this comment.
This code is reached by a call to survey.to_json so it shouldn't affect form conversion to XML.
Aha, thank you, that's great to know.
| name_of_main_section: str = None, | ||
| sections: Dict[str, Dict] = None, | ||
| main_section: Dict[str, Any] = None, | ||
| id_string: Optional[str] = None, |
There was a problem hiding this comment.
id_string seems like it shouldn't be optional.
There was a problem hiding this comment.
I sprinkled on this type annotation to help understand data flow. The id_string (and perhaps other params) could be mandatory, but then it seemed to make sense to refactor the body of the function to remove is None checks, etc., which could easily snowball into more refactoring.
| # Reference to list name for data dictionary tools (ilri/odktools). | ||
| question["list_name"] = list_name | ||
| # Copy choices for data export tools (onaio/onadata). | ||
| # TODO: could onadata use the list_name to look up the list for |
There was a problem hiding this comment.
One challenge with not having a documented API for pyxform at all is that it's not clear how to put things on a deprecation path. Did you happen to also look at Kobo for any direct usage of this kind?
There was a problem hiding this comment.
Various kobo repos (including kobocat, kpi, formpack) import pyxform. formpack uses workbook_to_json, but I didn't look in to how they use the data structures. formpack seems to be used by kpi, but I'm not sure how that relates to kobocat or other projects. kpi dependency spec has pyxform==1.9.0.
|
Ok, @lindsay-stevens, I'm ready to merge once the couple of things above are addressed! The only real blocker for me is that I'm concerned dropping |
|
@lognaturel thanks for the review! I have responded to the review comments. About the Enketo issue, I don't see where the pyxform output formatting could cause this. There's no spacing added to the output e.g. near the |
|
I have filed Enketo issues: enketo/enketo#29, enketo/enketo#28 The first is bad but I don't think it's bad enough to block merge. We can communicate the issues and encourage platforms that use pyxform and Enketo to update both at the same time. We will release this as |
|
We've identified a number of Enketo bugs as we have done quality assurance on this change. We're holding release until we can resolve those. |
|
All Enketo bugs we know about are fixed! 🎉 But now a user has reported this strange error. I can reproduce it on |
|
@lognaturel Great news about Enketo! I have opened a new PR to fix this error, which occurs when using or_other + translations + group or repeat. |
Closes enketo/enketo#216
Interesting git history from 2012: a change to itemsets for everything, then reverted the next day for backwards compatibility.
Why is this the best possible solution? Were any other approaches considered?
Discussed in enketo/enketo#216 and references. Lots of changes required but generally explained in commit messages or new docstrings.
What are the regression risks?
Risk that some XForm consuming apps may rely on selects not always generating itemsets. Risk that since there are a lot of code changes there may be bugs, although tests were added / updated.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
It's not a user-facing so probably not.
Before submitting this PR, please make sure you have:
testsnosetestsand verified all tests passblack pyxform teststo format code