Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not warn on unlabeled field-lists #542

Closed
wants to merge 8 commits into from

Conversation

yanokwa
Copy link
Contributor

@yanokwa yanokwa commented Jul 3, 2021

Too many warnings make it hard for users to see things they should be actually warned about. In this case, users use field-lists to visually group questions and that grouping really doesn't need a label, so we don't need to warn users.

And while I was looking at the warnings, I wanted to improve them by specifying that the warning was about a group or a repeat.

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

Originally, I wanted to warn on every group and every repeat, but decided to dial it back because...

  • Collect uses the repeat label in the UI when asking about what creating repeats
  • Groups without a field-list are probably fine to remove a warning, but this current PR feels like a safer first step.

What are the regression risks?

There could be some downstream tools that rely on group labels, but since has always been a warning and not an error, it seems likely that those tools can handle missing group labels.

There doesn't seem to be a fixed list of acceptable ways to say "begin group". There is a regex in https://github.com/XLSForm/pyxform/blob/master/pyxform/xls2json.py#L570 that I built my tests around.

Finally, there isn't a lot of test coverage on groups, so that didn't make me feel great.

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

No, infact https://xlsform.org/en/#multiple-webpage-forms shows an example of a field-list without a label.

Before submitting this PR, please make sure you have:

  • included test cases for core behavior and edge cases in tests_v1
  • run nosetests and verified all tests pass
  • run black pyxform to format code
  • verified that any code or assets from external sources are properly credited in comments

@yanokwa yanokwa changed the title Do not warn on unlabeled groups or repeats Do not warn on unlabeled field-lists Jul 3, 2021
@yanokwa yanokwa force-pushed the no-warning-unlabeled-group branch 4 times, most recently from a2beca0 to 941fc08 Compare July 4, 2021 03:44
@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2021

Codecov Report

Merging #542 (941fc08) into master (cf7ba39) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 941fc08 differs from pull request most recent head 9ce6a38. Consider uploading reports for the commit 9ce6a38 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #542      +/-   ##
==========================================
+ Coverage   83.88%   83.93%   +0.04%     
==========================================
  Files          25       25              
  Lines        3736     3741       +5     
  Branches      872      873       +1     
==========================================
+ Hits         3134     3140       +6     
+ Misses        455      454       -1     
  Partials      147      147              
Impacted Files Coverage Δ
pyxform/aliases.py 100.00% <100.00%> (ø)
pyxform/constants.py 100.00% <100.00%> (ø)
pyxform/xls2json.py 78.68% <100.00%> (+0.15%) ⬆️
pyxform/validators/util.py 77.45% <0.00%> (+0.30%) ⬆️

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...9ce6a38. Read the comment docs.

@lognaturel
Copy link
Contributor

Is this ready for a look or any particular reason it's still a draft?

lindsay-stevens added a commit to lindsay-stevens/pyxform that referenced this pull request Jul 28, 2021
- label check block applies to questions and other item types such as
  groups and repeats, so may be valid in other scenarios
- for example this block is referenced in XLSForm#542
- will require further changes to exclude question items from the check
  since that is the intent in XLSForm#439
@yanokwa yanokwa marked this pull request as ready for review July 28, 2021 16:01
@yanokwa yanokwa requested a review from lognaturel July 28, 2021 16:01
@lognaturel
Copy link
Contributor

@lindsay-stevens and I discussed and are on board. He had to make some conflicting changes in #439 so this will need to be reworked some.

@yanokwa yanokwa force-pushed the no-warning-unlabeled-group branch 2 times, most recently from b0ccab6 to 0ce77b0 Compare July 28, 2021 20:25
lindsay-stevens and others added 8 commits July 28, 2021 13:28
- as per pyxform/XLSForm#439, should always output a label node even if there
  is no label specified.
- pyxform had a warning for no label but this is removed since there is
  no requirement for a label.
- add tests to verify that a label is output and no warning raised
- fix existing tests that either expected a warning or no label
- some clients hide the guidance hint by default, in which case the user
  would see nothing / a small (i) symbol if a question had only a
  guidance hint without any label (or media) or hint
- update test_guidance_hint.py tests accordingly
- change newly added test from ss_structure to md-style
- removed e2e tests and associated files; more or less the same as the
  test immediately above in test_sheet_columns.py
- label check block applies to questions and other item types such as
  groups and repeats, so may be valid in other scenarios
- for example this block is referenced in XLSForm#542
- will require further changes to exclude question items from the check
  since that is the intent in XLSForm#439
- as noted in the comment, the check doesn't apply to questions anymore
  since PR XLSForm#543 makes pyxform always emit a label, and the only case
  that is not permissible is a guidance_hint with no label or hint.
- nesting this check inside the "control group" processing branch seems
  the only neat+easy way about this, since:
  - the context is a loop from which the various branches continue, so
    it can't check at the end,
  - each branch parses / determines the actual item type which is not
    apparent in the raw "type" data from the row dict, and
  - after the loop context exits, the data is in an array with nested
    dicts so it's no longer possible to determine the sheet row number
    for the error message to be as useful to users.
- refactored an old test which covers a lot of question types, into
  the tests_v1 / md style since this test is affected by the changes
  and having md style is more transparent
- added some tests to verify the assumption that for the label check,
  it's not going to encounter a None or empty value for "control_type".
@yanokwa yanokwa force-pushed the no-warning-unlabeled-group branch from 0ce77b0 to 9ce6a38 Compare July 28, 2021 20:32
@yanokwa
Copy link
Contributor Author

yanokwa commented Jul 28, 2021

Closing in favor of #545

@yanokwa yanokwa closed this Jul 28, 2021
@yanokwa yanokwa deleted the no-warning-unlabeled-group branch July 28, 2021 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants