733: Accept a list of references in the trigger column#776
733: Accept a list of references in the trigger column#776lognaturel merged 2 commits intoXLSForm:masterfrom
Conversation
- not much change but main differences since last update: - run with py3.12 instead of py3.10 - xml character escaping changes for py3.13 compatibility
- main topics:
- consolidate more of the pyxform parsing with new trigger parsing
- add "maybe_strip" to avoid making new strings if possible
- add error codes framework with optional extra context
- parsing/expression.py
- re-use inner lexer rule parts for compiled regexes used elsewhere
- add capturing group names for clearer usage (rather than index)
- parsing/instance_expression.py
- swap to consolidated regex
- add pre-check for "instance(" to skip expensive parsing if possible
- parameters_generic.py
- search set maybe slightly faster than search list
- fix type annotation since "allowed" is used by sorted()
- pyxform_reference.py
- collect existing pyxform reference helpers into this module
- add _parse func for dealing with common parse/validate steps
- question_types.py
- simplify exsting validators
- add parse_trigger for handling possibly comma-separated list of refs
- builder.py
- adapt `_save_trigger` to assume there now may be multiple trigger
values, and those value(s) are already processed.
- errors.py
- add new ErrorCodes for consolidating the location and language used
in error / warning messages. Has the option for adding more context
variables when an error is caught, otherwise "unknown" is used for
message tokens without a value.
- question.py
- move active implementation of get_setvalue_node_for_dynamic_default
to Question (from SurveyElement) since only a Question has a
"default" property (e.g. Survey, Section, or Option would always
return None because they do not have a "default")
- survey.py
- in .xml(), remove `triggering_reference` parse check since this is
already done earlier via xls2json, and again later by insert_xpaths
when doing the final string processing.
- in .get_trigger_values_for_question_name(), look up the reference
target name as-is, since that is how `builder.py` stores it now.
- in _generate_last_saved_instance() update name of has_last_saved
- add .get_element_by_name() to handle the lookup from the internal
property `._xpath` and the associated error states.
- in _var_repl_function()
- update match object references to use capturing group names
- update function body to use get_element_by_name() and to call
element.get_xpath() only once.
- in insert_xpaths(), add a docstring for the parameters, as best can
be determined at this stage (may not be 100% accurate but at least
the gist of it should be more useful than just the param name).
- survey_element.py
- use `has_pyxform_reference` instead of older regex
- utils.py
- remove pyxform regexes now covered by pyxform_reference.py
- xls2json.py
- use maybe_strip and is/has_pyxform_reference
- implement new multiple reference triggers
| "${a},${b}", # single comma | ||
| "${a},,${b}", # double comma | ||
| "${a},${b},", # trailing comma | ||
| "${a},${b},,", # trailing double comma |
There was a problem hiding this comment.
It would be nice to have an explicit case for a comma followed by whitespace because that feels like it will be common. If I understand correctly, that will be handled by stripping whitespace around the reference.
lognaturel
left a comment
There was a problem hiding this comment.
This is going to be so useful! And with some nice quality of life improvements as well. 👍
|
Since my only suggestion is so tiny, I'm going ahead and merging so this can get exercised on https://getodk.org/xlsform/ and so conflicts in the newest PRs can get addressed (sorry!) |
|
I just discovered something unfortunate -- using a comma-separated list of references in the |
|
All the tests looked good so I hadn't tried this and I don't think it works, surprisingly! I've filed an issue at #779 |
Closes #733
Why is this the best possible solution? Were any other approaches considered?
Implements the spec per #733 while attempting to consolidate more of the pyxform reference variable parsing code (further details in main commit message).
The current code actively enforces a comma delimiter to meet the spec in #733 but this check could be removed. A comma delimiter is unnecessary because the
${}token format is already a kind of delimiter (as opposed to parameters which arek1=v1, k2=v2); the existing parsing already handles finding multiple references in a string (such as for inserting<output/>).What are the regression risks?
There is some internal refactoring to consolidate reference parsing, for instance deleting the
BRACKETED_TAG_REGEXwhich has been around for a long while.Does this change require updates to documentation? If so, please file an issue here and include the link below.
Yes, to let users know they can use multiple triggers, and how to use them / possible use cases (docs ticket).
Before submitting this PR, please make sure you have:
testspython -m unittestand verified all tests passruff format pyxform testsandruff check pyxform teststo lint code