XLSForm / pyxform

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

clean up dependencies, py2 code, etc 🚿 #562

Closed lindsay-stevens closed 2 years ago

lindsay-stevens commented 2 years ago

Closes #558

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

This PR includes a range of janitorial changes intended to make maintenance easier:

What are the regression risks?

Pyxform v1.0.0 (2020-02-06) removed Python 2 compatibility, but due to a lot of compatibility stuff being left in place it's possible that some are still using Pyxform on py2. This PR makes it almost certain that Pyxform will not work on py2.

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

As mentioned above, the README was updated where relevant. No changes needed to user XLSForm docs.

Before submitting this PR, please make sure you have:

codecov-commenter commented 2 years ago

Codecov Report

Merging #562 (f46afd3) into master (94af8bc) will increase coverage by 0.86%. The diff coverage is 84.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #562      +/-   ##
==========================================
+ Coverage   83.36%   84.22%   +0.86%     
==========================================
  Files          25       25              
  Lines        3738     3665      -73     
  Branches      875      863      -12     
==========================================
- Hits         3116     3087      -29     
+ Misses        482      439      -43     
+ Partials      140      139       -1     
Impacted Files Coverage Δ
pyxform/validators/enketo_validate/__init__.py 34.21% <ø> (ø)
pyxform/xls2xform.py 81.57% <ø> (-0.24%) :arrow_down:
pyxform/xform2json.py 79.37% <45.45%> (-0.05%) :arrow_down:
pyxform/survey_element.py 94.40% <60.00%> (+0.32%) :arrow_up:
pyxform/xform_instance_parser.py 71.91% <66.66%> (-0.32%) :arrow_down:
pyxform/validators/updater.py 83.00% <80.00%> (-0.12%) :arrow_down:
pyxform/xls2json.py 79.49% <85.18%> (+3.17%) :arrow_up:
pyxform/utils.py 91.33% <88.88%> (+5.02%) :arrow_up:
pyxform/xls2json_backends.py 76.33% <88.88%> (-0.14%) :arrow_down:
pyxform/builder.py 77.07% <100.00%> (-0.12%) :arrow_down:
... and 10 more

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 94af8bc...f46afd3. Read the comment docs.

lognaturel commented 2 years ago

Have taken a quick look and don't see any issues. 👍 I'm hoping it will be a quick rebase on master to fix conflicts. I'll finish the review today or tomorrow.

lindsay-stevens commented 2 years ago

resolved merge conflicts

lindsay-stevens commented 2 years ago

Great, thanks @lognaturel !

About the tests and blame, the moves were tracked (git mv) so they should still show up in blame. At least, they do for me - both on CLI and GitHub when browsing the PR branch. One exception being 1750d59 where I copied the XFormTestCase definition lines into a different file. But we can still go to the parent commit and blame from there if needed.

About the conflict PRs, I agree it would be simpler to proceed with this PR and resolve conflicts in other PRs; than to try to resolve them first then incorporate into this PR.

About squashing, I don't mind either way. Many of the commits are noisy but some have potentially useful context.

lognaturel commented 2 years ago

they do for me - both on CLI and GitHub when browsing the PR branch

That's excellent! I didn't try here, I've had trouble with Github following moves in the past but maybe either the way you did it or changes on Github's side made this better. Either way, fantastic!

Many of the commits are noisy but some have potentially useful context.

I don't find them noisy and I'd tend to want to keep them but I think they'd be hard to use because some strange things happened in merges and rebases and they are all duplicated. My experience is also that it's hard to navigate history through multiple layers of merge commits. Seems like it will be most useful in the long term to squash and refer back to this PR if more context is needed. I don't think there's a quick way to get a clean history with just the desired commits.