785: remove warning for group (or loop) without label #801
Conversation
This file contains hidden or 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
- xls2json.py:
- replace "is" (identity) operator with "==" as they're not equivalent
- since the label check now only applies to "repeats", specify that.
- move type check to the first in the list to avoid checking other
test expressions unnecessarily for group/loop
- remove irrelevant membership check of aliases.label_optional_types
since this only applies to questions
- remove aliases.label_optional_types which is now unused
- remove irrelevant check of "calculation" and "default" (via the
_dynamic_default) since these columns are not applicable to repeats
- add positive/negative tests for groups/loops with and without labels,
translations, and appearance. The appearance check is to avoid a
regression since appearance used to be a factor in the label check.
- among the loop tests there's no cases for unlabelled choice-based
groups since currently choice labels are required by the loop
generation code in builder.py
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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 #785
Why is this the best possible solution? Were any other approaches considered?
See commit message for details. Generally simplifies the label check code, applies the change to both groups and loops, and improves test coverage to help avoid regressions about this requirement in future.
As mentioned in the commit message, as a follow up (or addition to this PR), the requirement for choice labels used as the source for loops could also be relaxed. I haven't implemented that at this stage since I'm not sure if that is desirable (it probably is - labels aren't usually required for choices and now won't be for groups or loops).
What are the regression risks?
If any users or other libraries expect this warning to be there, it won't.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
It might be OK to leave docs as-is with regard to recommending group labels, but if anywhere explicitly says they're required then that should be updated (ODK docs, xlsform.org, or XLSForm template, kapa knowledge base, etc).
Before submitting this PR, please make sure you have:
testspython -m unittestand verified all tests passruff format pyxform testsandruff check pyxform teststo lint code