search

XLSForm / pyxform

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

Add support for csv-external #522

Closed lognaturel closed 3 years ago

lognaturel commented 3 years ago

Closes #271

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

Uses the same code paths as xml-external which reduces risk. I briefly considered treating them differently in some way but that seemed more complex for no payoff.

I tried to come up with the minimal tests we needed to validate the addition. Since the same code is used as with xml-external, I think a lot of the cases are already handled but please be on the lookout for anything that might not be covered.

I noticed some tests were configured to always run Validate. We've discussed not doing that to speed up the test suite so I removed that. We always run all tests with Validate before a release. I also added a commit to make sure all tests pass Validate because failures were mostly in the test file I was already in.

What are the regression risks?

This is an additive change and I don't see a regression risk. The only existing code that has changed has to do with xml-external and that has really good test coverage.

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

https://github.com/XLSForm/xlsform.github.io/issues/216

Before submitting this PR, please make sure you have:

codecov-io commented 3 years ago

Codecov Report

Merging #522 (c52b5af) into master (bee9654) will increase coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #522   +/-   ##
=======================================
  Coverage   83.92%   83.92%           
=======================================
  Files          25       25           
  Lines        3713     3714    +1     
  Branches      865      865           
=======================================
+ Hits         3116     3117    +1     
  Misses        452      452           
  Partials      145      145
Impacted Files Coverage Δ
pyxform/question_type_dictionary.py 57.14% <ø> (ø)
pyxform/builder.py 77.18% <100.00%> (ø)
pyxform/survey.py 92.53% <100.00%> (+0.01%) :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 bee9654...dd036ed. Read the comment docs.

lognaturel commented 3 years ago

We should do jr://file-csv/name.csv

I completely forgot about that, you're absolutely right! 😬

lognaturel commented 3 years ago

Thanks for remembering that, @MartijnR. I'm a little shaken that https://github.com/XLSForm/pyxform/pull/522/files#diff-8ca5f58100688d382c2625d49faf9b321a0edd1f4dd98526bc8e71d0f11ab4e8R176 didn't catch that. I was thinking it would help us know the generated instance declarations would be identical. I now see there's only a check for an instance name conflict so it doesn't really cover us that much.

lognaturel commented 3 years ago

I rebased on master which auto fixed merge conflicts.