Detect incomplete XPath comparisons - #861
Open
ukanga wants to merge 1 commit into
Open
Conversation
Contributor
|
Thanks for filing this, @ukanga. I think it's worth doing and the tests look reasonable. My immediate reaction is that it feels like the validation should be possible to do with an existing pass over fields that can have expressions. @lindsay-stevens can you please take a look when you get a chance? |
lognaturel
removed their request for review
August 20, 2026 22:07
lognaturel
reviewed
Aug 20, 2026
| ), | ||
| None, | ||
| ) | ||
| == "OPS_COMP" |
Contributor
There was a problem hiding this comment.
Why would dangling comparison operators be more likely than any other dangling operator?
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
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 #860
Why is this the best possible solution? Were any other approaches considered?
Expressions ending with a comparison operator are incomplete, but pyxform
currently emits them into the generated XForm when external validation is
disabled. The resulting error is only reported later by ODK Validate or a form
engine, without the original XLSForm sheet, row, and column information.
This change adds targeted validation before XForm generation. It uses the
existing expression lexer to inspect the final non-whitespace token and raises a
PyXFormErrorwhen that token is a comparison operator.The validation covers:
relevant,constraint,calculation,required,and
readonly.choice_filter, repeat count, and dynamic defaults.instance_nameexpression.entity_id,create_if,update_if, andlabelexpressions.names in error messages.
It runs when using both
convert(validate=False)andxls2xform --skip_validate.The targeted lexer check addresses the identified error while leaving broader
XPath validation unchanged.
What are the regression risks?
The main risk is incorrectly rejecting literal values containing comparison
operator characters. This is mitigated by checking lexer tokens rather than
string suffixes.
Tests verify that:
There are no public API or type-signature changes. The intentional behavior
change is that previously emitted invalid expressions now raise a
PyXFormError, including when external validation is skipped.Does this change require updates to documentation? If so, please file an issue here and include the link below.
No documentation updates are required. This is an internal validation
improvement for an already invalid expression.
AI/LLM assistance
This PR was developed with assistance from a coding agent and LLM, including
implementation, tests, and supporting text. The submitted changes remain the
responsibility of the contributor.
Before submitting this PR, please make sure you have:
testspython -m unittestand verified all tests passruff format pyxform testsandruff check pyxform teststo lint code