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

No warning unlabeled group 2 #545

Merged
merged 2 commits into from
Aug 31, 2021

Conversation

yanokwa
Copy link
Contributor

@yanokwa yanokwa commented Jul 28, 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.

I also removed the warning for username and start-geopoint. The later, I'm not sure ever threw a warning.

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 mentioned this pull request Jul 28, 2021
4 tasks
@yanokwa yanokwa force-pushed the no-warning-unlabeled-group-2 branch from d469d3a to b295f3f Compare July 28, 2021 20:52
@yanokwa yanokwa force-pushed the no-warning-unlabeled-group-2 branch from 8eee72f to 551dc7f Compare July 28, 2021 21:05
@yanokwa yanokwa requested a review from lognaturel July 28, 2021 21:07
@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2021

Codecov Report

Merging #545 (551dc7f) into master (2e631b1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #545   +/-   ##
=======================================
  Coverage   83.85%   83.85%           
=======================================
  Files          25       25           
  Lines        3747     3748    +1     
  Branches      875      875           
=======================================
+ Hits         3142     3143    +1     
  Misses        457      457           
  Partials      148      148           
Impacted Files Coverage Δ
pyxform/aliases.py 100.00% <ø> (ø)
pyxform/xls2json.py 78.56% <ø> (ø)
pyxform/constants.py 100.00% <100.00%> (ø)

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 2e631b1...551dc7f. Read the comment docs.

@lognaturel lognaturel merged commit fe9118f into XLSForm:master Aug 31, 2021
@yanokwa yanokwa deleted the no-warning-unlabeled-group-2 branch August 31, 2021 17:23
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.

3 participants