clean up dependencies, py2 code, etc 🚿 #562
Conversation
- only xlrd and unicodecsv used by prod code - updated dev dependencies from pip-compile in clean virtualenv
- black's default is 88 - this results in a bit of reformatting - also resolve some code analysis warnings in xls2json.py: - prefix __main__ variables to avoid shadowing from outer scope - remove unreachable statement and unused variable
- also try to fix occasional failure from test_validator_update.py which is due to 'port already in use'
- separates prod code from test code to make prod dependencies clearer - removes unnecessary test code and files from the release package - makes it easier to search prod vs test code
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
- only necessary for python2 which is no longer supported
- for python2 to help unicode compatibility, no longer needed
- to quarantine them from other tests until they can be refactored, so they are not used as an example / template for new tests
- importerror - trailing white space - unused variables - ambiguous variable name
- don't need to do this in for unicode strings in py3
- not obvious that it was broken before but the line with "if len(cascading_choices)" refers to a variable that was deleted in the referenced commit. Didn't show up as a broken variable reference until after code recently reformatted with updated black version - no tests hit this code branch, probably since it requires a question which is named using "cascading_command" or "cascading_level" as per the regexp which is removed in this commit.
- results in one formatting change
- for cross-platform, need to specify newlines=""
- add python version badge - update source install instructions for py3 way of doing virtualenvs - add details / clarifications to development section - dev requirements not needed for building package
|
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. |
- black's default is 88 - this results in a bit of reformatting - also resolve some code analysis warnings in xls2json.py: - prefix __main__ variables to avoid shadowing from outer scope - remove unreachable statement and unused variable
- only necessary for python2 which is no longer supported
- for python2 to help unicode compatibility, no longer needed
- to quarantine them from other tests until they can be refactored, so they are not used as an example / template for new tests
- importerror - trailing white space - unused variables - ambiguous variable name
- don't need to do this in for unicode strings in py3
- not obvious that it was broken before but the line with "if len(cascading_choices)" refers to a variable that was deleted in the referenced commit. Didn't show up as a broken variable reference until after code recently reformatted with updated black version - no tests hit this code branch, probably since it requires a question which is named using "cascading_command" or "cascading_level" as per the regexp which is removed in this commit.
- results in one formatting change
- for cross-platform, need to specify newlines=""
- add python version badge - update source install instructions for py3 way of doing virtualenvs - add details / clarifications to development section - dev requirements not needed for building package
- highlighted by code inspection noting that the assertRaises 2nd arg is an unexpected type. That warning is wrong because the annotated type for the 2nd kwarg is "Any". But actually there are problems with these tests, as described below. - test_two_options_cannot_have_same_value raises an exception but it is actually due to the Question name having a space. Since there's a setting to allow choice duplicates, the check is done at a higher level than the survey elements. The intended test case is covered by another test, in test_fields.test_duplicate_fields_diff_cases. - test_one_section_cannot_have_two_conflicting_slugs raises two exceptions, first with the name having a space (as above) and second the InputQuestions do no have a "type" kwarg (e.g. text, select1, etc). The intended test case is covered by another test, in test_fields.test_duplicate_choices_without_setting.
|
resolved merge conflicts |
There was a problem hiding this comment.
I went through commit by commit and validated that the action made sense. I also did a quick check for each commit to make sure that it performs the action described by the commit message and does so correctly. Thanks for separating out the commits -- this would have been impossible to do any kind of reasonable review of otherwise.
There are some duplicate commits that were introduced in merges and rebases. Though the history was very helpful in review, I think we should squash this. I'll give a day or two for comments before doing that.
The two things that I did some thinking about are
- Moving all the tests. It's unfortunate that
blamewon't provide anything useful anymore. I find it occasionally useful to find out when and why a test was introduced and that will be much harder. But it's probably not a huge deal. - @KeynesYouDigIt has two open PRs that I never seem to be able to get a chance to review. This will cause conflicts. I propose we still merge this first and we can help with conflicts when reviewing.
| Make sure to include tests for the changes you're working on. When writing new tests you should add them in `pyxform/test_v1` instead of in `pyxform/test` (which contains tests using an older style). Generally, the easiest way to write tests is to extend `PyxformTestCase` which will let you compile example XLSForm and make assertions on the resulting XForm. | ||
| Make sure to include tests for the changes you're working on. When writing new tests you should add them in `tests` folder. Add to an existing test module, or create a new test module. Test modules are named after the corresponding source file, or if the tests concern many files then module name is the topic or feature under test. | ||
|
|
||
| When creating new test cases, where possible use `PyxformTestCase` as a base class instead of `unittest.TestCase`. The `PyxformTestCase` is a toolkit for writing XLSForms as MarkDown tables, compiling example XLSForms, and making assertions on the resulting XForm. This makes code review much easier by putting the XLSForm content inline with the test, instead of in a separate file. A `unittest.TestCase` may be used if the new tests do not involve compiling an XLSForm (but most will). Do not add new tests using the old style `XFormTestCase`. |
There was a problem hiding this comment.
Great paragraph. Thanks for explaining this so clearly.
# Conflicts: # tests/test_allow_mock_accuracy.py
|
Great, thanks @lognaturel ! About the tests and blame, the moves were tracked ( 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. |
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!
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. |
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:
pip install pyxformpackage (wheel/sdist)setup.cfgso that:blackupdatetestsroot folder so it's easier to grep / search codetestsandtests_v1merged into one folder because they're all relevantxform_test_casesub-folder__future__, conditional imports, usage ofunicodecsvandunittest2unicodeorbasestringtostr, andunichrtochrflake8,pycodestyle, pycharmWhat 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:
tests_v1nosetestsand verified all tests passblack pyxformto format code