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 xforms version #430

Merged
merged 6 commits into from
Feb 14, 2020
Merged

Add xforms version #430

merged 6 commits into from
Feb 14, 2020

Conversation

MartijnR
Copy link
Contributor

@MartijnR MartijnR commented Feb 13, 2020

closes #393

@MartijnR
Copy link
Contributor Author

@lognaturel, there is one pesky test that outputs odk:xforms-version="" instead of odk:xforms-version="1.0.0". Do you perhaps have an idea to could put me in the right direction to fix this?

@lognaturel
Copy link
Contributor

lognaturel commented Feb 13, 2020

Taking a look now! Those XForm2json tests are really frustrating. Do you know of anyone using that code to attempt to parse XForms to get JSON?

Also, it looks like this branch as pushed fails several tests. I had to rebase on master to get to the single failure you're describing.

@lognaturel
Copy link
Contributor

lognaturel commented Feb 13, 2020

At a high-level, I imagine that xform2json.py needs to explicitly save the value of that attribute when it parses an XForm. You probably figured that. Digging deeper...

Edit: ah, except that there's nowhere to save that value in the XLSForm. So no, that's not it. It's the json representation that needs to have that attribute set, maybe?

The XForms version is being treated as a setting and needs to be moved from a model attribute to a setting in the json representation built from XML.

@lognaturel
Copy link
Contributor

lognaturel commented Feb 13, 2020

lognaturel@f392273 is a fix.

This PR introduces a xforms_version setting. See this form for a sample form that sets the version to 7000. This is a bit strange because it's just passed through with no modification to the output. That is, it's likely to be a lie. On the other hand, depending on the nature of changes to future ODK XForms versions, a user could possibly indicate the intended way of interpreting a form they author? Do we really want to expose this?

@MartijnR
Copy link
Contributor Author

Thank you so much!

Sorry, I messed up the branch. Indeed, it shouldn't be an XLSForm setting!

@MartijnR MartijnR force-pushed the feature/xforms-version-393 branch from 7ced541 to 870950e Compare February 14, 2020 17:20
@codecov-io
Copy link

codecov-io commented Feb 14, 2020

Codecov Report

Merging #430 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #430      +/-   ##
==========================================
+ Coverage   82.72%   82.73%   +0.01%     
==========================================
  Files          23       23              
  Lines        3386     3389       +3     
  Branches      786      786              
==========================================
+ Hits         2801     2804       +3     
  Misses        439      439              
  Partials      146      146
Impacted Files Coverage Δ
pyxform/constants.py 100% <100%> (ø) ⬆️
pyxform/survey.py 91.4% <100%> (+0.03%) ⬆️

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 8dfd800...437c9d3. Read the comment docs.

@MartijnR MartijnR changed the title WIP: Feature/xforms version 393 Add xforms version Feb 14, 2020
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.

We took a meandering path to get there but this now looks really great to me! Thanks for bearing with my thinking through the alternatives, @MartijnR. I think we've ended up with a reasonable solution.

@MartijnR
Copy link
Contributor Author

Thank you! Turned out to be just a simple change, but it got me to explore the exciting world of pyxform. Ready for my next assignment! (No, I'm joking, I need a lengthy rest first!)

@lognaturel lognaturel merged commit b7474d8 into master Feb 14, 2020
@lognaturel lognaturel deleted the feature/xforms-version-393 branch February 14, 2020 19:19
@lognaturel
Copy link
Contributor

I need a lengthy rest first!

pyxform makes me feel like that a lot, too. It's definitely a heavy thing maintaining old code bases when the original masterminds aren't around. But everything we do here adds a lot of value to users so that keeps me going.

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.

Output no-repeat-reference-error in some manner or the pyxform version or the xforms version
3 participants