Repository navigation
863: add expression delimiter syntax - #870
Conversation
- Does not appear to effect the grammar behaviour since the test cases in tests/parsing/test_expression.py identify these terminals (e.g. SELF_REF) but the backslash is still a typo / unnecessary. - A backslash is automatically added when pressing enter inside a multi-line f-string in pycharm so that's probably how it was added.
- not related to expression.py specifically
- the actual parsing happens imperatively in other modules based on the tokens from this function, such as in instance_expression.py
- the _setup_xpath_dictionary function indexes all child SurveyElements into one list, and thereby validates that names are not re-used. - this is a validation task and the result could be useful to other functions prior to calling the final `xml()` output steps function.
- accept an expression wrapped in double curly braces as a string
delimiter to wrap in an `output` element.
- allows wider range of expressions than was possible with previous
detection of `instance()` calls or ${} variable references.
- move the logic for variable resolution and fallback parsing over to
the new func, to simplify survey.py and testing
- remove maybe_strip of space chars in expression in order to match old
behaviour of variable resolution - the simpler thing seems to be to
not add spaces in `Survey._var_repl_function` but that may have other
consequences, and it breaks a lot of tests, so it is left for a later
changeset.
- add test cases for delimited expressions in various unit scenarios
- test usage works in survey/choices translatable text/media columns.
- test escaped delims in text, instance() fallback behaviour
- use 'variable references' name since they're called 'variables' in the docs, rather than 'pyxform references', and it seems distinct enough from expressions to not have to differentiate further. - move parsing funcs to new parsing module - rename existing validation module - move `resolve_variables` func from expression_delimited.py into parsing/variable_reference.py
There was a problem hiding this comment.
Very thorough tests! One case I tried as a sanity check is
md = """
| survey |
| | type | name | label |
| | text | q1 | Result: {{ ${invalid name} + 1 }} |
"""
Indeed, the malformed reference is detected and errors out as desired.
I indicated a few things we need to explicitly document.
| """Should attempt to process `instance()` expressions if no expression delimiter found.""" | ||
|
|
||
| def test_ignored_when__delimited_expression(self): | ||
| """Should ignore instance() in text adjacent to delimited expression.""" |
There was a problem hiding this comment.
Should be explicitly documented
| def test_escaped_closing_delimiter_inside_expression(self): | ||
| """Should accept escaped closing delimiter inside an expression.""" | ||
| # EDB01 EDB05 | ||
| inp = r"Result: {{ concat('foo', '\}}') }}" |
| def test_triple_delimiter_tokens(self): | ||
| """Should ignore extra delimiter tokens and accept outer pair of delimiter tokens.""" | ||
| # EDB01 EDB08 EDB14 | ||
| inp = "Triple open: {{{ ${q1} }}}" |
There was a problem hiding this comment.
It might be more useful to a form designer to error on extra { or }? Maybe it's just not that likely but clients would likely give ugly crashes on those extra curly braces.
I had a similar thought about empty {{ }}. That's just passed through and becomes an empty output currently but it could also be an error since the form designer probably forgot something.
There was a problem hiding this comment.
I added some tests in dd68d02 to clarify behaviour. Finding the extra delimiter character as part of this sequence might involve differentiating it from braces in variable references ${}, which might end up pulling that grammar in (which is pretty big for NCNAME). Or maybe the ${} refs could be resolved before the {{}} refs pass. Also I wondered about whether whitespace would be potentially intentional or significant for such an error e.g. {{ { ${q1} } }}. Maybe we can revisit if it causes problems? There are many other ways that an expression inside these delimiters could be invalid XPath for clients but pyxform leaves full parsing to Validate.
There was a problem hiding this comment.
Though in c8b7701 I added an error for empty/blank expressions since that is a more obviously invalid scenario that is also straightforward to catch.
I think it's unlikely enough that people have {{ }} to just go ahead. But we can announce the feature on the forum and get feedback on that once it's merged and can be tried on the staging server.
It's contained enough that I think we could keep it indefinitely. |
- at some point in grammar development these were significant, but now if the rule is matched then it's OK to return the processed string - added two test cases to expand on existing adjacent tests with a similar structure, to clarify that these cases don't raise an error.
- for the empty/blank case, assume that a user forgot to add an expression there and raise an error. There doesn't seem to be a use case for an empty output node. - since the internal errors raised by expression_delimited.py end up being shown to the user, make the phrasing a bit friendlier. - add expected exception check for each error test parsing assertion, they might differ from assertPyxformXform e.g. if the error is caught and wrapped in an error that says which row/column has a problem.
b7717c2 to
c8b7701
Compare
lindsay-stevens
changed the title
2 participants
Closes #704
Closes #844
Closes #863
Why is this the best possible solution? Were any other approaches considered?
Design discussed in #863. There are a few related re-organisation commits but the main changes are in fc19c5e.
Considered expanding existing
instance()detection but the problem is accurately finding where the expression ends, seeing as it can be any valid XPath 1.0 expression (with ODK extensions). And anyway the new delimiter syntax allows for a much wider range of XPath expressions besidesinstance().A current limitation is that the processing happens in the Survey module, so parsing error messages aren't as nice as they could be. There's no exact row/column references or suggestion help text, the messages just say something went wrong and echo back the problem string. This could be improved in a follow-up PR.
What are the regression risks?
Strings which contain (escaped) delimiter syntax in the relevant survey/choices text/media columns are now processed as if they use the new feature. This could be mitigated via a (temporary?) form-level setting to opt-out of the new expression delimiter processing. The existing
instance()processing is retained (for now?) to reduce the urgency of updating existing forms.Does this change require updates to documentation? If so, please file an issue here and include the link below.
Yes
Before submitting this PR, please make sure you have:
testspython -m unittestand verified all tests passruff format pyxform testsandruff check pyxform teststo lint code