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

Add support for csv-external #522

Merged
merged 3 commits into from
Mar 26, 2021
Merged

Conversation

lognaturel
Copy link
Contributor

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.

XLSForm/xlsform.github.io#216

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

@codecov-io
Copy link

codecov-io commented Feb 25, 2021

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%) ⬆️

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.

ukanga
ukanga previously approved these changes Feb 26, 2021
Copy link
Contributor

@MartijnR MartijnR left a comment

Choose a reason for hiding this comment

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

Very exciting feature! Thanks!

We should do jr://file-csv/name.csv and this what we use with pulldata transformation in pyxform as well.

https://getodk.github.io/xforms-spec/#file-endpoints

@lognaturel
Copy link
Contributor Author

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

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

@lognaturel
Copy link
Contributor Author

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 lognaturel requested a review from MartijnR February 26, 2021 23:10
MartijnR
MartijnR previously approved these changes Mar 1, 2021
Copy link
Contributor

@MartijnR MartijnR left a comment

Choose a reason for hiding this comment

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

Thanks!

@lognaturel
Copy link
Contributor Author

I rebased on master which auto fixed merge conflicts.

@lognaturel lognaturel merged commit f4c76e7 into XLSForm:master Mar 26, 2021
@lognaturel lognaturel deleted the csv-external branch March 26, 2021 20:54
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.

Support csv-external for defining an external instance not necessarily used for a select
4 participants