647: include all choice lists in output #736
Merged
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
- no formatting changes, just keeping pace with ruff really
- allows users to include reference data for expressions via choices
without needing a hidden dummy question. Includes all choices rather
than parsing expressions to find references, because it seems unlikely
that both a XLSForm would include completely irrelevant choices and
have so many such choices that the XForm document size is a problem.
- test_xls2json_xls.py
- moved loop test into test_loop.py and modified to use the same style
as the existing test
- test_loop.py
- use xls2xform.convert rather than internals directly
- xls2json.py:
- move choices checks to new validator module
- include row reference in all errors/warnings. To have a consistent
error message structure, each duplicate choice gets a copy of the
message (in one error) rather than being smushed into one message.
- xlsparseutils.py
- replace separate regex for xml tags with expression parser and use
that instead, for consistent parsing rules and to use cache
- move the remaining sheet misspellings check to new validator module
- expression.py
- fix issue where a name containing "or" or "and" was parsed as an
operator rather than a name. These tokens are only valid when
surrounded by spaces so the regex is updated accordingly.
- add tests for positive and negative match cases for tag names.
lognaturel
approved these changes
Nov 1, 2024
| ) | ||
| INVALID_LABEL = ( | ||
| "[row : {row}] On the 'choices' sheet, the 'label' value is invalid. " | ||
| "Choices should have a label. " |
There was a problem hiding this comment.
This is optional/should because a choice list might now just be used for reference and not for a select?
There was a problem hiding this comment.
The reason for this check being a "should" / warning is to match existing behaviour - historically choice labels aren't required, but that might change per #534
There was a problem hiding this comment.
Ohhh, thank you for introducing me to my past self. :D
|
I agree this can close #630 and we can continue allowing : in the middle of an identifier. |
Hidden selects Begone! :-) +1 |
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 #647
Closes #630
Why is this the best possible solution? Were any other approaches considered?
What are the regression risks?
Does this change require updates to documentation? If so, please file an issue here and include the link below.
I don't think so. The problem scenario for the choices sheet is probably not relevant to most users, and I don't think the dummy question workaround was documented (except on the forum?) but for those who wanted it to work and had to go looking for the workaround, well now it just works. As for the leading colon in the name, xlsform.org says what characters names can contain and colon isn't one of them.
Before submitting this PR, please make sure you have:
testspython -m unittestand verified all tests passruff format pyxform testsandruff check pyxform teststo lint code