694: search and pulldata secondary instance conflict #697
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
- search appearance was recently changed to use a secondary instance, to avoid the issue of missing translations. However the secondary instance can result in name clashes. This can be avoided by changing the search() select's list name to something that doesn't clash, but anyway the secondary instance is redundant so it is now not emitted. - for two unsupported use cases, an error is now raised: - search appearance on a select_from_file - search appearance choice list used in other selects - add: tests for interactions with other ways of triggering secondary instances e.g. pulldata, external xml/csv, select_from_file, search. - chg: add constant for "list_name" and replace strings with it.
- add: tests for some settings that had no tests before, or were not well tested, or used XLS / JSON fixtures - chg: move NSMAP constant into constants module
lognaturel
approved these changes
Feb 23, 2024
I don't think either of these has reasonable expected behavior and errors are an improvement. 👍 |
lognaturel
reviewed
Feb 23, 2024
lognaturel
reviewed
Feb 23, 2024
Filed settings docs issue at XLSForm/xlsform.github.io#266 |
lindsay-stevens
force-pushed
the
pyxform-694
branch
from
199a6dc
to
e008ffd
Compare
|
Thanks! |
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 #694
Why is this the best possible solution? Were any other approaches considered?
The previous solution for search + translations was pretty close, but created the possibility of conflicts in the secondary instances. This PR removes the secondary instance that would have been emitted. It also raises an error two search feature combinations that are not supported, and even if they were, would be broken by this PR's strategy (see tests for examples, where
errored=True). These combinations are: 1) search appearance on a select_from_file, 2) search appearance choice list used in other selects.An alternative detailed in #694 was to merge compatible secondary instances, but @lognaturel clarified offline that this wouldn't work because the
srcattribute is how Collect/Enketo identify external instances. So having choices in a secondary instances with a non-blanksrcwouldn't work.What are the regression risks?
If users had forms which included the above 2 scenarios which now raise an error, they would need to fix up their form. If the error seems to harsh maybe a warning could be emitted, but ODK Validate raises an ugly error for these cases anyway (due to missing instance).
Does this change require updates to documentation? If so, please file an issue here and include the link below.
I tidied up and included some settings tests I wrote for recent forum issues. These settings could be included in the XLSForm reference template.
Before submitting this PR, please make sure you have:
testspython -m unittestand verified all tests passruff format pyxform testsandruff check pyxform teststo lint code