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

Apply section name check to sections only #529

Merged

Conversation

jnm
Copy link
Member

@jnm jnm commented Apr 8, 2021

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 Change error message if section name is equal to form name #511, losing a valuable addition to pyxform and ignoring the problem described in Proposal to change duplicate section name error message if equal to root node name #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:

  • 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

@jnm jnm requested a review from lognaturel April 8, 2021 17:20
@lognaturel
Copy link
Contributor

Indeed, thank you! I had missed @pbowen-oc's #510 (comment) 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?

…to `tests_v1`. Previously, acceptance of this was only checked in the
older `tests`
@jnm
Copy link
Member Author

jnm commented Apr 8, 2021

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

lognaturel
lognaturel previously approved these changes Apr 9, 2021
Copy link
Contributor

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏 Thank you! Some people I know would be very offended by the lack of explicit assertion and I might add errored=False to placate them but I don't mind as-is.

Leaving open for a bit in case @gushil, @MartijnR or @pbowen-oc want to sanity check. Any urgency to release?

@codecov-io
Copy link

Codecov Report

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

❗ 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
Copy link
Member Author

jnm commented Apr 9, 2021

I might add errored=False

Aye, aye 👍 I don't want to get run out of town or anything 😆

Any urgency to release?

Nope, none at all.

@MartijnR
Copy link
Contributor

MartijnR commented Apr 9, 2021

Fine with me. Thanks! 👍

@pbowen-oc
Copy link

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

@lognaturel lognaturel merged commit a539996 into XLSForm:master Apr 13, 2021
@jnm jnm deleted the apply-section-name-check-to-sections-only branch April 13, 2021 22:57
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.

5 participants