XLSForm / pyxform

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

Apply section name check to sections only #529

Closed jnm closed 3 years ago

jnm commented 3 years ago

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

511 is a helpful solution to #510, providing less-confusing error text when a group name matches the root node name, but it also ensnared (unintentionally?) regular question names. Other approaches considered and rejected were:

  1. Reverting #511, losing a valuable addition to pyxform and ignoring the problem described in #510;
  2. Doing nothing, and thus requiring unnecessary changes to forms that worked with pyxform prior to v1.3.4.

What are the regression risks?

Some unknown tool could depend on the recently-added check that prevents question names from equaling the form name.

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

No; however, if this PR is undesirable, then the documentation should be updated to reflect the new restriction on question names.

Before submitting this PR, please make sure you have:

lognaturel commented 3 years ago

Indeed, thank you! I had missed @pbowen-oc's https://github.com/XLSForm/pyxform/issues/510#issuecomment-763063318 to that effect.

Could you please add a test very similar to https://github.com/XLSForm/pyxform/pull/511/files#diff-2f87b2f8ddd9e41e8ebf5bf45472a415f5d2744e146a487aa0516762c79c47cf that verifies that a field name that matches the root node name is accepted?

jnm commented 3 years ago

Sure :) I added one just now. Should I also check the generated XML, or is it enough as-is? I started writing that but threw it away, thinking that it'd add brittleness and duplicate other tests

codecov-io commented 3 years ago

Codecov Report

Merging #529 (d606704) into master (43ea039) will not change coverage. The diff coverage is 50.00%.

:exclamation: Current head d606704 differs from pull request most recent head 3e37c32. Consider uploading reports for the commit 3e37c32 to get more accurate results Impacted file tree graph

@@           Coverage Diff           @@
##           master     #529   +/-   ##
=======================================
  Coverage   83.94%   83.94%           
=======================================
  Files          25       25           
  Lines        3719     3719           
  Branches      867      867           
=======================================
  Hits         3122     3122           
  Misses        452      452           
  Partials      145      145           
Impacted Files Coverage Δ
pyxform/survey.py 92.54% <50.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 43ea039...3e37c32. Read the comment docs.

jnm commented 3 years ago

I might add errored=False

Aye, aye :+1: I don't want to get run out of town or anything :laughing:

Any urgency to release?

Nope, none at all.

MartijnR commented 3 years ago

Fine with me. Thanks! 👍

pbowen-oc commented 3 years ago

@lognaturel - We would like this update, but there is no urgency on it.