812: Leading and trailing spaces in parameters lead to form error#845
Merged
Conversation
- move parsing from validators module to new parsing module
- change from regex etc to a lark grammar, which is easier to read
and could support enhancements like quoted parameter values to
handle values containing delimiters e.g. "a = 'b c', x = y".
- add test module for structural parsing rules
- delete misc tests that duplicate this e.g. in
test_parameters_rows.py the case "rows=" is a parse failure
- move existing error messages into errors.py and include row number
- update tests accordingly to use newer error check test pattern
- add helpers to enum.py / StrEnum to simplify src/test code
- move inline parameters lists from xls2json.py to constants.py
- eventually these may make more sense to live with the data currently
in question_type_dictionary.py but that is a bigger refactor
- delete module-level constants that were the same as parameter keys
added the prior commit, update usages accordingly
- change existing string constant usages of parameter keys in the source
tree to use the parameter enums
- there are still some string usages of parameter names in test,
such as test_range.py but the test tree was left alone to make
this refactor easier to trust, the test usages can be cleaned up
in a subsequent pass
lognaturel
approved these changes
Jun 12, 2026
lognaturel
left a comment
lognaturel
left a comment
Contributor
There was a problem hiding this comment.
Nice! Couple of questions that could prompt action but overall this looks ready to merge and build on.
| ], | ||
| ) | ||
|
|
||
| def test_throwing_error_when_blank_android_package_name_is_used_with_supported_appearances( |
Contributor
There was a problem hiding this comment.
Is this now just handled generically as a blank parameter? I have read this test a hundred times and I can't seem to convince myself it had any value so maybe that's why you removed it!
Contributor
Author
There was a problem hiding this comment.
Yes this test was deleted because the case is a generic parsing failure, now handled by test_parameters.py (though that only had the first one case - added the second one in 9fdd143)
| accepted=qt_params, | ||
| row_number=row_number, | ||
| ) | ||
| if "value" in parameters: |
Contributor
There was a problem hiding this comment.
Any particular reason not to introduce constants here and for label?
Contributor
Author
There was a problem hiding this comment.
Just missed this bit (and also text param `rows) - fixed in 9fdd143
- review feedback on prior two commits
- missed some parameters (value, label, row) in the conversions to
using the constants module instead of loose strings
- a deleted test case for the "app" parameter included a case where
the value after the equals is whitespace, so added that case
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #812
Why is this the best possible solution? Were any other approaches considered?
As outlined in the commit messages:
Also goes some way towards #592 in the sense of organising how parameters are handled and tested. I think next steps there could look like 1) moving the parameter handling or validation code blocks out of xls2json.py into a different module(s) or classes, and 2) upgrading the error messages to the new pattern that includes the row number; but these seemed like deviating too far from the motivating issue #812.
What are the regression risks?
Primarily I think the changes to
constants.py, but that module is not exported at the project level__init__.py. I otherwise made minimal compatibility changes to the existing tests to try and avoid regressions.Does this change require updates to documentation? If so, please file an issue here and include the link below.
Not directly, but I noticed that the current XLSForm reference template is missing many accepted parameters (perhaps intentionally). Hopefully collecting the parameter keys in one place will now make it easier to keep the docs in sync.
Before submitting this PR, please make sure you have:
testspython -m unittestand verified all tests passruff format pyxform testsandruff check pyxform teststo lint code