602: preserve order of columns when building secondary instance #672
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
- Seems like main reason for sorting in the first place was to help satisfy string-based matching for XML output tests. Since there are XPath test facilities now, it's not necessary. - fix: remove explicit sorting in survey.py and utils.py - fix: remove order randomisation in xls2json.py via conversion of keys to set objects - use dicts instead so that order is preserved. - fix: tests broken by the above changes - fix: add retry loop for shutting down server between test runs. - add: test asserting column order for additional choices columns
lognaturel
approved these changes
Dec 1, 2023
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 #602
Why is this the best possible solution? Were any other approaches considered?
Seems like main reason for sorting in the first place was to help satisfy string-based matching for XML output tests. Since there are XPath test facilities now, it's not necessary. These changes remove explicit sorting in
survey.pyandutils.py, and remove order randomisation inxls2json.pythat happened via conversion of keys to set objects - it now uses dicts instead so that order is preserved.There is still a bit of sorting going on in
pyxform_test_case.pyandxform_test_case.pybut since that's purely for testing it would not affect #602.What are the regression risks?
If XForm consumers (such as Collect or Enketo or other tools) expected the sorted behaviour, those consumers would have to sort elements instead of expecting the form elements to be pre-sorted. Similarly, pyxform library users would need to apply a sort if needed e.g. in the survey choices dict.
It's possible that some tests might randomly fail either in CI or later on, because of the string matching thing. In which case it would only be a failure related to element or attribute order. I ran the test suite about a dozen times to try and catch them all.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No I would say this is an internal implementation detail. The spec doesn't say the elements should be sorted.
Before submitting this PR, please make sure you have:
testsnosetestsand verified all tests passblack pyxform teststo format code