XLSForm / pyxform

A Python package to create XForms for ODK Collect.
BSD 2-Clause "Simplified" License
77 stars 134 forks source link

Output empty `<label/>` instead of omitting it. #543

Closed lindsay-stevens closed 2 years ago

lindsay-stevens commented 2 years ago

Closes #439

Why is this the best possible solution? Were any other approaches considered?

As discussed in the issue ticket, the XForms spec requires a label node. It would not be feasible to change the spec, so this solution brings pyxform output in to line with the spec.

What are the regression risks?

It's possible that existing users expect there to be a warning if no label is supplied but a hint is supplied. So the regression risk would be in other apps that use pyxform. In this case the other app could add a check for empty labels.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

The xlsform.org site says that the survey sheet label is mandatory. However, the update here is specific to pyxform behaviour. It would not be accurate to say that the label is not mandatory, because it is. It's just that for pyxform, in the absence of a label it will emit one in order to comply with the spec.

Before submitting this PR, please make sure you have:

lognaturel commented 2 years ago

From @MartijnR at https://github.com/XLSForm/pyxform/issues/439#issuecomment-867815645:

If ODK Validate does not get fooled by an empty label (and still requires either a label or hint text content)

The PR description doesn't address this that I can see -- will Validate still throw an error if there's an empty label and no hint? That would be the desired behavior.

lognaturel commented 2 years ago

I've come back to try this, and I do confirm that there is still a pyxform error thrown if neither label nor hint is present which is what we want. 👍

I went straight to tests to try to see if that was covered and didn't see it because the forms aren't inline. Also, I now see that test_row_without_label_or_calculation_throws_error already existed and remains which is great.

lindsay-stevens commented 2 years ago

will Validate still throw an error if there's an empty label and no hint?

For this case there is an existing test: in test_sheet_columns.InvalidSurveyColumnsTests.test_missing_label: https://github.com/XLSForm/pyxform/blob/6bb547b409f46f78bb54af398b6da7e869ed418b/pyxform/tests_v1/test_sheet_columns.py#L36

For reference, the two e2e spreadsheets look like this: hint_with_no_label_column: hint_with_no_label_column hint_with_no_label_value: hint_with_no_label_value

codecov-commenter commented 2 years ago

Codecov Report

Merging #543 (a0d3f7c) into master (cf7ba39) will decrease coverage by 0.03%. The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #543      +/-   ##
==========================================
- Coverage   83.88%   83.85%   -0.04%     
==========================================
  Files          25       25              
  Lines        3736     3747      +11     
  Branches      872      875       +3     
==========================================
+ Hits         3134     3142       +8     
- Misses        455      457       +2     
- Partials      147      148       +1     
Impacted Files Coverage Δ
pyxform/survey_element.py 94.07% <76.92%> (-1.00%) :arrow_down:
pyxform/xls2json.py 78.56% <100.00%> (+0.03%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update cf7ba39...a0d3f7c. Read the comment docs.

lindsay-stevens commented 2 years ago

Hi @lognaturel, resolving this turned out to be not as simple as restoring the check code block. When it was restored, a couple of tests needed to be updated. In particular there is an old-style test with a large "warnings.xls" fixture, which is now refactored into markdown. This should make it clearer when the label warning will and will not be raised. From here, #542 could add in the new condition to exclude warnings for field-list groups, and add the new tests. More context / info in the 3e4a84e commit message.