785: remove warning for group (or loop) without label#801
785: remove warning for group (or loop) without label#801lindsay-stevens merged 1 commit intoXLSForm:masterfrom
Conversation
- 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
lognaturel
left a comment
lognaturel
left a comment
There was a problem hiding this comment.
One additional small opportunity for simplification.
| constants.LABEL not in row | ||
| control_type == constants.REPEAT | ||
| and constants.LABEL not in row | ||
| and row.get(constants.MEDIA) is None |
There was a problem hiding this comment.
Repeats can't have media associated with them so we could get rid of this one too.
There was a problem hiding this comment.
I think this might be one of those things like ${instanceId} where it seems obvious (to me as well) but might need some further discussion that is outside the scope of this PR so I've filed this issue.
| control_type == constants.REPEAT | ||
| and constants.LABEL not in row | ||
| and row.get(constants.MEDIA) is None | ||
| and question_type not in aliases.label_optional_types |
There was a problem hiding this comment.
These checks were copy and pasted from non-control checks presumably? I didn't check but I assume they exist and are tested in the place that would actually make sense for them.
There was a problem hiding this comment.
Yes originally this conditional block applied to all rows in the survey sheet, then it was moved down a few lines to only apply to container controls, and was on the way refined a few times down to the current state. I think for non-container controls there's no check in xls2json but SurveyElement.xml_label_and_hint catches it. The latest related issues for that seem to be #790 and #334
There's no documentation for loops currently and I've never seen them used. Let's hold off on follow-up work for now!
Pretty sure not, will keep looking, though! |
|
Thanks! |
2 participants
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