From c009938d0a3e1e2786d194300338ac932b23e93e Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Thu, 11 Jun 2026 15:30:07 +1000 Subject: [PATCH 1/3] chg: allow spaces around parameter equals delimiter - 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 --- pyxform/constants.py | 61 +++++++++++++ pyxform/errors.py | 15 ++++ pyxform/parsing/parameters.py | 55 ++++++++++++ pyxform/util/enum.py | 10 ++- pyxform/validators/pyxform/parameters.py | 26 ++++++ .../validators/pyxform/parameters_generic.py | 49 ---------- pyxform/validators/pyxform/question_types.py | 13 ++- pyxform/xls2json.py | 76 +++++++++------- tests/parsing/test_parameters.py | 89 +++++++++++++++++++ tests/test_allow_mock_accuracy.py | 19 +++- tests/test_external_instances_for_selects.py | 30 ++++--- tests/test_geo.py | 44 ++++++--- tests/test_image_app_parameter.py | 34 ++----- tests/test_parameters_rows.py | 2 +- tests/test_randomize_itemsets.py | 10 ++- tests/test_range.py | 16 ++-- 16 files changed, 402 insertions(+), 147 deletions(-) create mode 100644 pyxform/parsing/parameters.py create mode 100644 pyxform/validators/pyxform/parameters.py delete mode 100644 pyxform/validators/pyxform/parameters_generic.py create mode 100644 tests/parsing/test_parameters.py diff --git a/pyxform/constants.py b/pyxform/constants.py index 88091de56..76981ec90 100644 --- a/pyxform/constants.py +++ b/pyxform/constants.py @@ -177,3 +177,64 @@ class EntityColumns(StrEnum): SUPPORTED_MEDIA_TYPES = {"image", "big-image", "audio", "video"} OR_OTHER_CHOICE = {NAME: "other", LABEL: "Other"} RESERVED_NAMES_SURVEY_SHEET = {META} + + +########################################################################################## +# Parameters +########################################################################################## +# Name enums by question type, or if shared then use a sensible question type prefix. +# For aliased question types, use the primary documented name e.g. "image" not "photo". +# The module question_type_dictionary.py handles default parameter values, if any. +# Add new enums or keys alphabetical order. +class ParametersAudio(StrEnum): + QUALITY = "quality" + + +class ParametersAudit(StrEnum): + IDENTIFY_USER = "identify-user" + LOCATION_MAX_AGE = "location-max-age" + LOCATION_MIN_INTERVAL = "location-min-interval" + LOCATION_PRIORITY = "location-priority" + TRACK_CHANGES = "track-changes" + TRACK_CHANGES_REASONS = "track-changes-reasons" + + +class ParametersGeo(StrEnum): + ALLOW_MOCK_ACCURACY = "allow-mock-accuracy" + INCREMENTAL = "incremental" + + +class ParametersGeoPoint(StrEnum): + ALLOW_MOCK_ACCURACY = "allow-mock-accuracy" + CAPTURE_ACCURACY = "capture-accuracy" + WARNING_ACCURACY = "warning-accuracy" + + +class ParametersImage(StrEnum): + APP = "app" + MAX_PIXELS = "max-pixels" + + +class ParametersRange(StrEnum): + END = "end" + PLACEHOLDER = "placeholder" + START = "start" + STEP = "step" + TICK_INTERVAL = "tick_interval" + TICK_LABELSET = "tick_labelset" + + +class ParametersSelect(StrEnum): + RANDOMIZE = "randomize" + SEED = "seed" + + +class ParametersSelectFromFile(StrEnum): + LABEL = "label" + RANDOMIZE = "randomize" + SEED = "seed" + VALUE = "value" + + +class ParametersText(StrEnum): + ROWS = "rows" diff --git a/pyxform/errors.py b/pyxform/errors.py index 60ecdb858..74cdb7778 100644 --- a/pyxform/errors.py +++ b/pyxform/errors.py @@ -506,6 +506,21 @@ class ErrorCode(Enum): "be 'true' or not included." ), ) + SURVEY_004: Detail = Detail( + name="Survey sheet - parameters parsing failed", + msg=( + "[row : {row}] On the 'survey' sheet, the 'parameters' value is invalid. " + "Parameters must be in the form of 'key1=value1 key2=value2." + ), + ) + SURVEY_005: Detail = Detail( + name="Survey sheet - parameters unknown key", + msg=( + "[row : {row}] On the 'survey' sheet, the 'parameters' value is invalid. " + "The accepted parameter keys for this question type are '{accepted}'. " + "The following are invalid parameter key(s): '{rejected}'." + ), + ) class PyXFormError(Exception): diff --git a/pyxform/parsing/parameters.py b/pyxform/parsing/parameters.py new file mode 100644 index 000000000..1696aead9 --- /dev/null +++ b/pyxform/parsing/parameters.py @@ -0,0 +1,55 @@ +from lark import Lark, Token, Transformer +from lark.exceptions import LarkError + +from pyxform.errors import ErrorCode, PyXFormError +from pyxform.parsing.expression import maybe_strip + +# Label and value are used to match against user-specified files so case should be preserved. +CASE_SENSITIVE_VALUES = {"label", "value"} + +PARAMETER_GRAMMAR = r""" + start: pair* + pair: TOKEN "=" TOKEN + // Anything that's not a delimiter. + TOKEN: /[^\s=,;]+/ + // Delimiters between key-value pairs. + %ignore /[\s,;]+/ +""" + +_PARAMETER_PARSER = Lark(PARAMETER_GRAMMAR, parser="lalr", start="start") + + +class ParameterTransformer(Transformer): + @staticmethod + def start(pairs: list[tuple[str, str]]) -> dict[str, str]: + """Combine (key, value) tuples into a dict""" + return dict(pairs) + + @staticmethod + def pair(items: list[Token, Token]) -> tuple[str, str]: + """Normalise matched (key, value) tokens.""" + raw_key, raw_value = items + key = maybe_strip(str(raw_key).lower()) + value = maybe_strip(str(raw_value)) + + if key not in CASE_SENSITIVE_VALUES: + value = value.lower() + + return key, value + + +# No token-specific (ALL_CAPS) methods, so visit_tokens=False. +_PARAMETER_TRANSFORMER = ParameterTransformer(visit_tokens=False) + + +def parse( + raw_parameters: str, + row_number: int, +) -> dict[str, str]: + if not raw_parameters or not raw_parameters.strip(): + return {} + + try: + return _PARAMETER_TRANSFORMER.transform(_PARAMETER_PARSER.parse(raw_parameters)) + except LarkError as e: + raise PyXFormError(code=ErrorCode.SURVEY_004, context={"row": row_number}) from e diff --git a/pyxform/util/enum.py b/pyxform/util/enum.py index 86d62976c..41bec2aef 100644 --- a/pyxform/util/enum.py +++ b/pyxform/util/enum.py @@ -28,5 +28,13 @@ def __new__(cls, *values): return member @classmethod - def value_list(cls): + def value_list(cls) -> list: return list(cls.__members__.values()) + + @classmethod + def value_set(cls) -> set: + return set(cls.__members__.values()) + + @classmethod + def value_str_sorted(cls) -> str: + return ", ".join(sorted(cls.__members__.values())) diff --git a/pyxform/validators/pyxform/parameters.py b/pyxform/validators/pyxform/parameters.py new file mode 100644 index 000000000..227343c5a --- /dev/null +++ b/pyxform/validators/pyxform/parameters.py @@ -0,0 +1,26 @@ +from typing import Any + +from pyxform.errors import ErrorCode, PyXFormError +from pyxform.util.enum import StrEnum + +PARAMETERS_TYPE = dict[str, Any] + + +def validate( + parameters: PARAMETERS_TYPE, + accepted: type[StrEnum], + row_number: int, +) -> dict[str, str]: + """ + Raise an error if 'parameters' includes any keys not named in 'accepted'. + """ + extras = set(parameters) - accepted.value_set() + if 0 < len(extras): + raise PyXFormError( + ErrorCode.SURVEY_005.value.format( + row=row_number, + accepted=accepted.value_str_sorted(), + rejected=", ".join(sorted(extras)), + ) + ) + return parameters diff --git a/pyxform/validators/pyxform/parameters_generic.py b/pyxform/validators/pyxform/parameters_generic.py deleted file mode 100644 index 4647aaa2b..000000000 --- a/pyxform/validators/pyxform/parameters_generic.py +++ /dev/null @@ -1,49 +0,0 @@ -from collections.abc import Collection -from typing import Any - -from pyxform.errors import PyXFormError -from pyxform.parsing.expression import maybe_strip - -PARAMETERS_TYPE = dict[str, Any] - -# Label and value are used to match against user-specified files so case should be preserved. -CASE_SENSITIVE_VALUES = {"label", "value"} - - -def parse(raw_parameters: str) -> PARAMETERS_TYPE: - parts = raw_parameters.split(";") - if len(parts) == 1: - parts = raw_parameters.split(",") - if len(parts) == 1: - parts = raw_parameters.split() - - params = {} - for param in parts: - kv_split = param.split("=") - if "=" not in param or len(kv_split) != 2: - raise PyXFormError( - "Expecting parameters to be in the form of " - "'parameter1=value parameter2=value'." - ) - k, v = kv_split - key = maybe_strip(k.lower()) - params[key] = v if key in CASE_SENSITIVE_VALUES else maybe_strip(v.lower()) - - return params - - -def validate( - parameters: PARAMETERS_TYPE, - allowed: Collection[str], -) -> dict[str, str]: - """ - Raise an error if 'parameters' includes any keys not named in 'allowed'. - """ - extras = set(parameters) - (set(allowed)) - if 0 < len(extras): - msg = ( - "Accepted parameters are '{a}'. " - "The following are invalid parameter(s): '{e}'." - ).format(a=", ".join(sorted(allowed)), e=", ".join(sorted(extras))) - raise PyXFormError(msg) - return parameters diff --git a/pyxform/validators/pyxform/question_types.py b/pyxform/validators/pyxform/question_types.py index 06386f790..51fbddd64 100644 --- a/pyxform/validators/pyxform/question_types.py +++ b/pyxform/validators/pyxform/question_types.py @@ -8,9 +8,13 @@ from typing import Any from pyxform import aliases +from pyxform import constants as co from pyxform.errors import ErrorCode, PyXFormError from pyxform.question_type_dictionary import QUESTION_TYPE_DICT -from pyxform.validators.pyxform import parameters_generic +from pyxform.validators.pyxform import parameters as pv +from pyxform.validators.pyxform.parameters import ( + PARAMETERS_TYPE, +) from pyxform.validators.pyxform.pyxform_reference import ( is_pyxform_reference_candidate, parse_pyxform_references, @@ -110,7 +114,7 @@ def validate_geo_parameter_incremental(value: str) -> None: def process_range_question_type( row_number: int, row: dict[str, Any], - parameters: parameters_generic.PARAMETERS_TYPE, + parameters: PARAMETERS_TYPE, appearance: str, choices: dict[str, Any], ) -> dict[str, Any]: @@ -119,9 +123,10 @@ def process_range_question_type( Raises PyXFormError when invalid range parameters are used. """ - parameters = parameters_generic.validate( + parameters = pv.validate( parameters=parameters, - allowed={"start", "end", "step", "tick_interval", "placeholder", "tick_labelset"}, + accepted=co.ParametersRange, + row_number=row_number, ) if ( appearance diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py index 0f992b876..1bfa12af5 100644 --- a/pyxform/xls2json.py +++ b/pyxform/xls2json.py @@ -24,6 +24,7 @@ ) from pyxform.errors import ErrorCode, PyXFormError from pyxform.parsing.expression import is_xml_tag +from pyxform.parsing.parameters import parse as parameters_parse from pyxform.parsing.sheet_headers import dealias_and_group_headers from pyxform.question_type_dictionary import get_meta_group from pyxform.utils import ( @@ -31,8 +32,9 @@ default_is_dynamic, print_pyobj_to_json, ) -from pyxform.validators.pyxform import parameters_generic, select_from_file, unique_names +from pyxform.validators.pyxform import parameters as pv from pyxform.validators.pyxform import question_types as qt +from pyxform.validators.pyxform import select_from_file, unique_names from pyxform.validators.pyxform import settings as validate_settings from pyxform.validators.pyxform.android_package_name import validate_android_package_name from pyxform.validators.pyxform.choices import validate_and_clean_choices @@ -144,8 +146,8 @@ def add_choices_info_to_question( question: dict[str, Any], list_name: str, choices: dict[str, list], - choice_filter: str, - file_extension: str, + choice_filter: str | None = None, + file_extension: str | None = None, ): """ Add choices-related info to the question dict, e.g. itemset, list_name, choices, etc. @@ -516,7 +518,10 @@ def workbook_to_json( action_module.ActionLibrary.setvalue_new_repeat.value.to_dict() ) - parameters = parameters_generic.parse(raw_parameters=row.get("parameters", "")) + parameters = parameters_parse( + raw_parameters=row.get("parameters", ""), + row_number=row_number, + ) # Pull out questions that will go in meta block if question_type == "audit": @@ -530,16 +535,10 @@ def workbook_to_json( row["name"] = "audit" new_dict = row.copy() - parameters_generic.validate( + pv.validate( parameters=parameters, - allowed=( - constants.LOCATION_PRIORITY, - constants.LOCATION_MIN_INTERVAL, - constants.LOCATION_MAX_AGE, - constants.TRACK_CHANGES, - constants.IDENTIFY_USER, - constants.TRACK_CHANGES_REASONS, - ), + accepted=constants.ParametersAudit, + row_number=row_number, ) if constants.TRACK_CHANGES in parameters: @@ -1034,16 +1033,18 @@ def workbook_to_json( new_json_dict = row.copy() new_json_dict[constants.TYPE] = select_type - select_params_allowed = ["randomize", "seed"] + select_params_allowed = constants.ParametersSelect if parse_dict["select_command"] in { "select_one_from_file", "select_multiple_from_file", }: - select_params_allowed += ["value", "label"] + select_params_allowed = constants.ParametersSelectFromFile # Look at parameters column for select parameters - parameters_generic.validate( - parameters=parameters, allowed=select_params_allowed + pv.validate( + parameters=parameters, + accepted=select_params_allowed, + row_number=row_number, ) if "randomize" in parameters: @@ -1177,7 +1178,11 @@ def workbook_to_json( if question_type == "text": new_dict = row.copy() - parameters_generic.validate(parameters=parameters, allowed=("rows",)) + pv.validate( + parameters=parameters, + accepted=constants.ParametersText, + row_number=row_number, + ) if "rows" in parameters: try: @@ -1199,12 +1204,10 @@ def workbook_to_json( if row.get("default"): new_dict["default"] = process_image_default(row["default"]) - parameters_generic.validate( + pv.validate( parameters=parameters, - allowed=( - "max-pixels", - "app", - ), + accepted=constants.ParametersImage, + row_number=row_number, ) if "max-pixels" in parameters: try: @@ -1239,7 +1242,11 @@ def workbook_to_json( if question_type == "audio": new_dict = row.copy() - parameters_generic.validate(parameters=parameters, allowed=("quality",)) + pv.validate( + parameters=parameters, + accepted=constants.ParametersAudio, + row_number=row_number, + ) if "quality" in parameters: if parameters["quality"] not in { @@ -1258,7 +1265,11 @@ def workbook_to_json( if question_type == "background-audio": new_dict = row.copy() - parameters_generic.validate(parameters=parameters, allowed=("quality",)) + pv.validate( + parameters=parameters, + accepted=constants.ParametersAudio, + row_number=row_number, + ) action = ( action_module.ActionLibrary.odk_recordaudio_instance_load.value.to_dict() ) @@ -1283,17 +1294,16 @@ def workbook_to_json( new_dict = row.copy() if question_type == "geopoint": - parameters_generic.validate( + pv.validate( parameters=parameters, - allowed=( - "allow-mock-accuracy", - "capture-accuracy", - "warning-accuracy", - ), + accepted=constants.ParametersGeoPoint, + row_number=row_number, ) else: - parameters_generic.validate( - parameters=parameters, allowed=("allow-mock-accuracy", "incremental") + pv.validate( + parameters=parameters, + accepted=constants.ParametersGeo, + row_number=row_number, ) if "allow-mock-accuracy" in parameters: diff --git a/tests/parsing/test_parameters.py b/tests/parsing/test_parameters.py new file mode 100644 index 000000000..3e8453e19 --- /dev/null +++ b/tests/parsing/test_parameters.py @@ -0,0 +1,89 @@ +from pyxform.errors import ErrorCode, PyXFormError +from pyxform.parsing.parameters import parse + +from tests.pyxform_test_case import PyxformTestCase + +# ( , , ) +param_positive = [ + ("Single pair", "value=val", {"value": "val"}), + ("Normalize keys to lowercase", "VALUE=val", {"value": "val"}), + ("Preserve case for 'value' value", "value=VAL", {"value": "VAL"}), + ("Preserve case for 'label' value", "label=Lb-L", {"label": "Lb-L"}), + ( + "Normalize values to lowercase for non-case-sensitive keys", + "other_key=VAL", + {"other_key": "val"}, + ), + ( + "Multiple pairs separated by space", + "value=val label=lbl", + {"value": "val", "label": "lbl"}, + ), + ( + "Multiple pairs separated by comma", + "value=val,label=lbl", + {"value": "val", "label": "lbl"}, + ), + ( + "Multiple pairs separated by semicolon", + "value=val;label=lbl", + {"value": "val", "label": "lbl"}, + ), + ( + "Whitespace around delimiters and equals", + " value = val1 , label = lbl1 ", + {"value": "val1", "label": "lbl1"}, + ), + ( + "Whitespace around delimiters and equals - motivating case from pyxform/#812", + "label = foo, value = bar", + {"label": "foo", "value": "bar"}, + ), + ( + "Special characters accepted in key or value", + "value=*val3#", + {"value": "*val3#"}, + ), + ( + "Duplicate keys have last-write-wins dict behavior", + "value=first value=second", + {"value": "second"}, + ), +] + +# ( , ) +param_negative = [ + ("Missing equals sign and value", "value"), + ("Missing key before equals", "=val"), + ("Missing value after equals", "value="), + ("Duplicate equals ", "value==val"), + ("Key with no value assignment", "value=val label"), + ("Spaces in values", "label=My Label"), + ("Invalid delimiter", "value=val&label=Label"), + ("No delimiter", "value=vallabel=Label"), +] + + +class TestParameterParser(PyxformTestCase): + def test_parse__positive(self): + """Should successfully parse structurally valid strings into matching dicts.""" + for description, case, expected in param_positive: + with self.subTest(case=case, description=description): + self.assertEqual(parse(case, row_number=1), expected) + + def test_parse__negative(self): + """Should raise a PyXFormError for structurally invalid parameters.""" + for description, case in param_negative: + with self.subTest(case=case, description=description): + with self.assertRaises(PyXFormError) as err: + parse(case, row_number=1) + self.assertEqual(err.exception.code, ErrorCode.SURVEY_004) + + def test_parse__empty_or_whitespace(self): + """Should return an empty dict on empty or blank inputs.""" + for description, case in [ + ("Empty string", ""), + ("Whitespace string", " "), + ]: + with self.subTest(case=case, description=description): + self.assertEqual(parse(case, row_number=1), {}) diff --git a/tests/test_allow_mock_accuracy.py b/tests/test_allow_mock_accuracy.py index bb58c821b..75679da7a 100644 --- a/tests/test_allow_mock_accuracy.py +++ b/tests/test_allow_mock_accuracy.py @@ -1,3 +1,6 @@ +from pyxform import constants as co +from pyxform.errors import ErrorCode + from tests.pyxform_test_case import PyxformTestCase @@ -190,7 +193,13 @@ def test_geoshape_with_accuracy_parameters_errors(self): | | geoshape | geoshape | Geoshape | warning-accuracy=5 | """, errored=True, - error__contains=["invalid parameter(s): 'warning-accuracy'"], + error__contains=[ + ErrorCode.SURVEY_005.value.format( + row=2, + accepted=co.ParametersGeo.value_str_sorted(), + rejected="warning-accuracy", + ), + ], ) def test_geotrace_with_accuracy_parameters_errors(self): @@ -202,5 +211,11 @@ def test_geotrace_with_accuracy_parameters_errors(self): | | geotrace | geotrace | Geotrace | warning-accuracy=5 | """, errored=True, - error__contains=["invalid parameter(s): 'warning-accuracy'"], + error__contains=[ + ErrorCode.SURVEY_005.value.format( + row=2, + accepted=co.ParametersGeo.value_str_sorted(), + rejected="warning-accuracy", + ), + ], ) diff --git a/tests/test_external_instances_for_selects.py b/tests/test_external_instances_for_selects.py index c559bd37a..782330152 100644 --- a/tests/test_external_instances_for_selects.py +++ b/tests/test_external_instances_for_selects.py @@ -332,12 +332,17 @@ def test_with_params_no_filters(self): | | select_one_external city | city | City | value=val, label=lbl | | | select_one_external suburb | suburb | Suburb | value=val, label=lbl | """ - err = ( - "Accepted parameters are 'randomize, seed'. " - "The following are invalid parameter(s): 'label, value'." - ) self.assertPyxformXform( - name="test", md=md + self.all_choices, errored=True, error__contains=[err] + name="test", + md=md + self.all_choices, + errored=True, + error__contains=[ + ErrorCode.SURVEY_005.value.format( + row=3, + accepted=co.ParametersSelect.value_str_sorted(), + rejected="label, value", + ), + ], ) def test_no_params_with_filters(self): @@ -394,12 +399,17 @@ def test_with_params_with_filters(self): | | select_one_external city | city | City | state=${state} | value=val, label=lbl | | | select_one_external suburb | suburb | Suburb | state=${state} and city=${city} | value=val, label=lbl | """ - err = ( - "Accepted parameters are 'randomize, seed'. " - "The following are invalid parameter(s): 'label, value'." - ) self.assertPyxformXform( - name="test", md=md + self.all_choices, errored=True, error__contains=[err] + name="test", + md=md + self.all_choices, + errored=True, + error__contains=[ + ErrorCode.SURVEY_005.value.format( + row=3, + accepted=co.ParametersSelect.value_str_sorted(), + rejected="label, value", + ), + ], ) def test_list_name_not_in_external_choices_sheet_raises_error(self): diff --git a/tests/test_geo.py b/tests/test_geo.py index 1b46acdad..e5d80874d 100644 --- a/tests/test_geo.py +++ b/tests/test_geo.py @@ -135,7 +135,7 @@ def test_with_incremental__geoshape_geotrace____wrong_value__error(self): | | {type} | q1 | Q1 | incremental={value} | """ types = ["geoshape", "geotrace"] - values = ["", "yeah", "false"] + values = ["yeah", "false"] for t in types: for v in values: with self.subTest((t, v)): @@ -149,23 +149,43 @@ def test_with_incremental__geoshape_geotrace____wrong_value__error(self): ], ) + def test_with_incremental__geopoint__error(self): + """Should raise an error if specified for geopoint.""" + md = """ + | survey | + | | type | name | label | parameters | + | | geopoint | q1 | Q1 | incremental=true | + """ + self.assertPyxformXform( + md=md, + errored=True, + error__contains=[ + ErrorCode.SURVEY_005.value.format( + row=2, + accepted=co.ParametersGeoPoint.value_str_sorted(), + rejected="incremental", + ), + ], + ) + def test_with_incremental__wrong_type_with_params__error(self): """Should raise an error if specified for other question types with parameters.""" md = """ | survey | | | type | name | label | parameters | - | | {type} | q1 | Q1 | incremental=true | + | | audio | q1 | Q1 | incremental=true | """ - types = ["geopoint", "audio"] - for t in types: - with self.subTest(t): - self.assertPyxformXform( - md=md.format(type=t), - errored=True, - error__contains=[ - "The following are invalid parameter(s): 'incremental'." - ], - ) + self.assertPyxformXform( + md=md, + errored=True, + error__contains=[ + ErrorCode.SURVEY_005.value.format( + row=2, + accepted=co.ParametersAudio.value_str_sorted(), + rejected="incremental", + ), + ], + ) def test_with_incremental__wrong_type_no_params__ok(self): """Should not raise an error if specified for other question types without parameters.""" diff --git a/tests/test_image_app_parameter.py b/tests/test_image_app_parameter.py index 3d8006e49..b6fad7da5 100644 --- a/tests/test_image_app_parameter.py +++ b/tests/test_image_app_parameter.py @@ -2,6 +2,9 @@ Test image max-pixels and app parameters. """ +from pyxform import constants as co +from pyxform.errors import ErrorCode + from tests.pyxform_test_case import PyxformTestCase @@ -48,31 +51,6 @@ def test_throwing_error_when_invalid_android_package_name_is_used_with_supported ], ) - def test_throwing_error_when_blank_android_package_name_is_used_with_supported_appearances( - self, - ): - appearances = ("", "annotate") - parameters = ("app=", "app= ") - md = """ - | survey | | | | | | - | | type | name | label | parameters | appearance | - | | image | my_image | Image | {parameter} | {appearance} | - """ - for appearance in appearances: - for parameter in parameters: - with self.subTest(msg=f"{appearance} - {parameter}"): - self.assertPyxformXform( - name="data", - errored=True, - error__contains=[ - "[row : 2] Parameter 'app' has an invalid Android package name - package name is missing." - ], - md=md.format(parameter=parameter, appearance=appearance), - xml__xpath_match=[ - "/h:html/h:body/x:upload[not(@intent) and @mediatype='image/*' and @ref='/data/my_image']" - ], - ) - def test_ignoring_invalid_android_package_name_with_not_supported_appearances( self, ): @@ -145,7 +123,11 @@ def test_string_extra_params(self): | | image | my_image | Image | max-pixels=640 foo=bar | """, error__contains=[ - "Accepted parameters are 'app, max-pixels'. The following are invalid parameter(s): 'foo'." + ErrorCode.SURVEY_005.value.format( + row=2, + accepted=co.ParametersImage.value_str_sorted(), + rejected="foo", + ), ], ) diff --git a/tests/test_parameters_rows.py b/tests/test_parameters_rows.py index 50c14be3d..943cc3fea 100644 --- a/tests/test_parameters_rows.py +++ b/tests/test_parameters_rows.py @@ -48,7 +48,7 @@ def test_adding_rows_to_the_body_if_set_in_parameters( def test_throwing_error_if_rows_set_in_parameters_but_the_value_is_not_an_integer( self, ): - parameters = ("rows=", "rows=foo", "rows=7.5") + parameters = ("rows=foo", "rows=7.5") md = """ | survey | | | | | | | type | name | label | parameters | diff --git a/tests/test_randomize_itemsets.py b/tests/test_randomize_itemsets.py index 3c464eb33..e2d97e350 100644 --- a/tests/test_randomize_itemsets.py +++ b/tests/test_randomize_itemsets.py @@ -2,6 +2,9 @@ Test randomize itemsets. """ +from pyxform import constants as co +from pyxform.errors import ErrorCode + from tests.pyxform_test_case import PyxformTestCase from tests.xpath_helpers.choices import xpc from tests.xpath_helpers.questions import xpq @@ -166,8 +169,11 @@ def test_randomized_select_one_bad_param(self): """, error__contains=[ - "Accepted parameters are 'randomize, seed'. " - "The following are invalid parameter(s): 'step'." + ErrorCode.SURVEY_005.value.format( + row=2, + accepted=co.ParametersSelect.value_str_sorted(), + rejected="step", + ), ], ) diff --git a/tests/test_range.py b/tests/test_range.py index d4396d4c4..e2a747ab4 100644 --- a/tests/test_range.py +++ b/tests/test_range.py @@ -34,6 +34,7 @@ - RB004: if the range has values that are decimals the data type must be 'decimal'. """ +from pyxform import constants as co from pyxform.errors import ErrorCode from tests.pyxform_test_case import PyxformTestCase @@ -71,8 +72,11 @@ def test_parameter_list__error(self): md=md, errored=True, error__contains=[ - "Accepted parameters are 'end, placeholder, start, step, tick_interval, tick_labelset'. " - "The following are invalid parameter(s): 'stop'." + ErrorCode.SURVEY_005.value.format( + row=2, + accepted=co.ParametersRange.value_str_sorted(), + rejected="stop", + ), ], ) @@ -151,7 +155,7 @@ def test_parameter_delimiter_invalid__error(self): md=md.format(sep=value), errored=True, error__contains=[ - "Expecting parameters to be in the form of 'parameter1=value parameter2=value'." + ErrorCode.SURVEY_004.value.format(row=2), ], ) @@ -171,9 +175,7 @@ def test_parameter_malformed__error(self): self.assertPyxformXform( md=md.format(name=name, value=value), errored=True, - error__contains=[ - "Expecting parameters to be in the form of 'parameter1=value parameter2=value'." - ], + error__contains=[ErrorCode.SURVEY_004.value.format(row=2)], ) def test_parameter_not_a_number__error(self): @@ -185,7 +187,7 @@ def test_parameter_not_a_number__error(self): | | range | q1 | Q1 | {name}={value} | """ params = ("start", "end", "step", "tick_interval", "placeholder") - cases = ("", "one") + cases = ("*", "one") for name in params: for value in cases: with self.subTest((name, value)): From 8c9f54909b6935a5c830204f21f84bca888e8828 Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Thu, 11 Jun 2026 17:18:51 +1000 Subject: [PATCH 2/3] chg: de-duplicate params constants, use params constants in src tree - 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 --- pyxform/constants.py | 8 - pyxform/question.py | 11 +- pyxform/question_type_dictionary.py | 16 +- pyxform/validators/pyxform/question_types.py | 70 ++-- pyxform/xls2json.py | 330 ++++++++++--------- 5 files changed, 239 insertions(+), 196 deletions(-) diff --git a/pyxform/constants.py b/pyxform/constants.py index 76981ec90..67e11597a 100644 --- a/pyxform/constants.py +++ b/pyxform/constants.py @@ -50,7 +50,6 @@ CONTROL = "control" APPEARANCE = "appearance" ITEMSET = "itemset" -RANDOMIZE = "randomize" CHOICE_FILTER = "choice_filter" PARAMETERS = "parameters" @@ -102,13 +101,6 @@ XLSX_EXTENSIONS = {".xlsx", ".xlsm"} SUPPORTED_FILE_EXTENSIONS = {*XLS_EXTENSIONS, *XLSX_EXTENSIONS} -LOCATION_PRIORITY = "location-priority" -LOCATION_MIN_INTERVAL = "location-min-interval" -LOCATION_MAX_AGE = "location-max-age" -TRACK_CHANGES = "track-changes" -IDENTIFY_USER = "identify-user" -TRACK_CHANGES_REASONS = "track-changes-reasons" - # supported bind keywords for which external instances will be created for pulldata function EXTERNAL_INSTANCES = {"calculate", "constraint", "readonly", "required", "relevant"} diff --git a/pyxform/question.py b/pyxform/question.py index 6dfb58e6b..8eb7f3c45 100644 --- a/pyxform/question.py +++ b/pyxform/question.py @@ -451,12 +451,17 @@ def build_xml(self, survey: "Survey"): if self.parameters: params = self.parameters - if "randomize" in params and params["randomize"] == "true": + if ( + constants.ParametersSelect.RANDOMIZE in params + and params[constants.ParametersSelect.RANDOMIZE] == "true" + ): nodeset = f"randomize({nodeset}" - if "seed" in params: + if constants.ParametersSelect.SEED in params: seed = maybe_strip( - survey.insert_xpaths(text=params["seed"], context=self) + survey.insert_xpaths( + text=params[constants.ParametersSelect.SEED], context=self + ) ) nodeset = f"{nodeset}, {seed}" diff --git a/pyxform/question_type_dictionary.py b/pyxform/question_type_dictionary.py index 66021a974..f3ee4e24c 100644 --- a/pyxform/question_type_dictionary.py +++ b/pyxform/question_type_dictionary.py @@ -6,7 +6,7 @@ from types import MappingProxyType from typing import Any -from pyxform import constants as const +from pyxform import constants as co _QUESTION_TYPE_DICT = { "q picture": { @@ -359,7 +359,11 @@ "range": { "control": {"tag": "range"}, "bind": {"type": "int"}, - "parameters": {"start": "1", "end": "10", "step": "1"}, + "parameters": { + co.ParametersRange.START.value: "1", + co.ParametersRange.END.value: "10", + co.ParametersRange.STEP.value: "1", + }, }, "audit": {"bind": {"type": "binary"}}, "xml-external": { @@ -390,8 +394,8 @@ def get_meta_group(children: Sequence[dict[str, Any]]) -> dict[str, Any]: if children is None: children = [] return { - const.NAME: "meta", - const.TYPE: const.GROUP, - const.CONTROL: {"bodyless": True}, - const.CHILDREN: children, + co.NAME: "meta", + co.TYPE: co.GROUP, + co.CONTROL: {"bodyless": True}, + co.CHILDREN: children, } diff --git a/pyxform/validators/pyxform/question_types.py b/pyxform/validators/pyxform/question_types.py index 51fbddd64..1d80f4124 100644 --- a/pyxform/validators/pyxform/question_types.py +++ b/pyxform/validators/pyxform/question_types.py @@ -132,7 +132,12 @@ def process_range_question_type( appearance and appearance not in {"vertical", "no-ticks"} and any( - k in parameters for k in ("tick_interval", "placeholder", "tick_labelset") + k in parameters + for k in ( + co.ParametersRange.TICK_INTERVAL, + co.ParametersRange.PLACEHOLDER, + co.ParametersRange.TICK_LABELSET, + ) ) ): raise PyXFormError(ErrorCode.RANGE_008.value.format(row=row_number)) @@ -160,44 +165,67 @@ def process_parameter(name: str) -> Decimal | None: ) return value - start = process_parameter(name="start") - end = process_parameter(name="end") - step = process_parameter(name="step") - tick_interval = process_parameter(name="tick_interval") - placeholder = process_parameter(name="placeholder") - tick_labelset = parameters.get("tick_labelset") + start = process_parameter(name=co.ParametersRange.START.value) + end = process_parameter(name=co.ParametersRange.END.value) + step = process_parameter(name=co.ParametersRange.STEP.value) + tick_interval = process_parameter(name=co.ParametersRange.TICK_INTERVAL.value) + placeholder = process_parameter(name=co.ParametersRange.PLACEHOLDER.value) + tick_labelset = parameters.get(co.ParametersRange.TICK_LABELSET) range_width = abs(end - start) if step == 0: - raise PyXFormError(ErrorCode.RANGE_002.value.format(row=row_number, name="step")) + raise PyXFormError( + ErrorCode.RANGE_002.value.format( + row=row_number, name=co.ParametersRange.STEP.value + ) + ) if step > range_width: - raise PyXFormError(ErrorCode.RANGE_003.value.format(row=row_number, name="step")) + raise PyXFormError( + ErrorCode.RANGE_003.value.format( + row=row_number, name=co.ParametersRange.STEP.value + ) + ) if tick_interval is not None: if tick_interval == 0: raise PyXFormError( - ErrorCode.RANGE_002.value.format(row=row_number, name="tick_interval") + ErrorCode.RANGE_002.value.format( + row=row_number, name=co.ParametersRange.TICK_INTERVAL.value + ) ) if tick_interval > range_width: raise PyXFormError( - ErrorCode.RANGE_003.value.format(row=row_number, name="tick_interval") + ErrorCode.RANGE_003.value.format( + row=row_number, name=co.ParametersRange.TICK_INTERVAL.value + ) ) if (tick_interval % step) != 0: raise PyXFormError( - ErrorCode.RANGE_004.value.format(row=row_number, name="tick_interval") + ErrorCode.RANGE_004.value.format( + row=row_number, name=co.ParametersRange.TICK_INTERVAL.value + ) ) - parameters["odk:tick-interval"] = parameters.pop("tick_interval") + # Input parameter uses underscore, output attribute name uses hyphen. + parameters["odk:tick-interval"] = parameters.pop( + co.ParametersRange.TICK_INTERVAL.value + ) if placeholder is not None: if (placeholder - start) % step != 0: raise PyXFormError( - ErrorCode.RANGE_004.value.format(row=row_number, name="placeholder") + ErrorCode.RANGE_004.value.format( + row=row_number, name=co.ParametersRange.PLACEHOLDER.value + ) ) if placeholder < min(start, end) or placeholder > max(start, end): raise PyXFormError( - ErrorCode.RANGE_005.value.format(row=row_number, name="placeholder") + ErrorCode.RANGE_005.value.format( + row=row_number, name=co.ParametersRange.PLACEHOLDER.value + ) ) - parameters["odk:placeholder"] = parameters.pop("placeholder") + parameters[f"odk:{co.ParametersRange.PLACEHOLDER.value}"] = parameters.pop( + co.ParametersRange.PLACEHOLDER.value + ) if tick_labelset: tick_list = choices.get(tick_labelset) @@ -219,11 +247,15 @@ def process_parameter(name: str) -> Decimal | None: raise PyXFormError(ErrorCode.RANGE_010.value.format(row=row_number)) if tick_interval is not None and (value - start) % tick_interval != 0: raise PyXFormError( - ErrorCode.RANGE_011.value.format(row=row_number, name="tick_interval") + ErrorCode.RANGE_011.value.format( + row=row_number, name=co.ParametersRange.TICK_INTERVAL.value + ) ) elif (value - start) % step != 0: raise PyXFormError( - ErrorCode.RANGE_011.value.format(row=row_number, name="step") + ErrorCode.RANGE_011.value.format( + row=row_number, name=co.ParametersRange.STEP.value + ) ) if no_ticks_appearance: no_ticks_labels.add(value) @@ -234,7 +266,7 @@ def process_parameter(name: str) -> Decimal | None: if no_ticks_labels != {start, end}: raise PyXFormError(ErrorCode.RANGE_012.value.format(row=row_number)) - parameters.pop("tick_labelset") + parameters.pop(co.ParametersRange.TICK_LABELSET.value) # Default is integer, but if the values have decimals then change the bind type. if any( diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py index 1bfa12af5..da5ca30e5 100644 --- a/pyxform/xls2json.py +++ b/pyxform/xls2json.py @@ -180,8 +180,9 @@ def add_choices_info_to_question( elif not ( # Select with randomized choices. ( - constants.RANDOMIZE in question[constants.PARAMETERS] - and question[constants.PARAMETERS][constants.RANDOMIZE] == "true" + constants.ParametersSelect.RANDOMIZE in question[constants.PARAMETERS] + and question[constants.PARAMETERS][constants.ParametersSelect.RANDOMIZE] + == "true" ) # Select from file e.g. type = "select_one_from_file cities.xml". or file_extension in EXTERNAL_INSTANCE_EXTENSIONS @@ -535,149 +536,133 @@ def workbook_to_json( row["name"] = "audit" new_dict = row.copy() + qt_params = constants.ParametersAudit pv.validate( parameters=parameters, - accepted=constants.ParametersAudit, + accepted=qt_params, row_number=row_number, ) - if constants.TRACK_CHANGES in parameters: - if ( - parameters[constants.TRACK_CHANGES] != "true" - and parameters[constants.TRACK_CHANGES] != "false" - ): + if qt_params.TRACK_CHANGES in parameters: + if parameters[qt_params.TRACK_CHANGES] not in {"true", "false"}: msg = ( - f"{constants.TRACK_CHANGES} must be set to true or false: " - f"'{parameters[constants.TRACK_CHANGES]}' is an invalid value." + f"{qt_params.TRACK_CHANGES.value} must be set to true or false: " + f"'{parameters[qt_params.TRACK_CHANGES]}' is an invalid value." ) raise PyXFormError(msg) else: new_dict["bind"] = new_dict.get("bind", {}) new_dict["bind"].update( { - "odk:" + constants.TRACK_CHANGES: parameters[ - constants.TRACK_CHANGES + f"odk:{qt_params.TRACK_CHANGES.value}": parameters[ + qt_params.TRACK_CHANGES ] } ) - if constants.TRACK_CHANGES_REASONS in parameters: - if parameters[constants.TRACK_CHANGES_REASONS] != "on-form-edit": + if qt_params.TRACK_CHANGES_REASONS in parameters: + if parameters[qt_params.TRACK_CHANGES_REASONS] != "on-form-edit": raise PyXFormError( - constants.TRACK_CHANGES_REASONS + " must be set to on-form-edit" + f"{qt_params.TRACK_CHANGES_REASONS.value} must be set to on-form-edit" ) else: new_dict["bind"] = new_dict.get("bind", {}) new_dict["bind"].update( - {"odk:" + constants.TRACK_CHANGES_REASONS: "on-form-edit"} + {f"odk:{qt_params.TRACK_CHANGES_REASONS.value}": "on-form-edit"} ) - if constants.IDENTIFY_USER in parameters: - if ( - parameters[constants.IDENTIFY_USER] != "true" - and parameters[constants.IDENTIFY_USER] != "false" - ): + if qt_params.IDENTIFY_USER in parameters: + if parameters[qt_params.IDENTIFY_USER] not in {"true", "false"}: msg = ( - f"{constants.IDENTIFY_USER} must be set to true or false: " - f"'{parameters[constants.IDENTIFY_USER]}' is an invalid value." + f"{qt_params.IDENTIFY_USER.value} must be set to true or false: " + f"'{parameters[qt_params.IDENTIFY_USER]}' is an invalid value." ) raise PyXFormError(msg) else: new_dict["bind"] = new_dict.get("bind", {}) new_dict["bind"].update( { - "odk:" + constants.IDENTIFY_USER: parameters[ - constants.IDENTIFY_USER + f"odk:{qt_params.IDENTIFY_USER.value}": parameters[ + qt_params.IDENTIFY_USER ] } ) location_parameters = { - constants.LOCATION_PRIORITY, - constants.LOCATION_MIN_INTERVAL, - constants.LOCATION_MAX_AGE, + qt_params.LOCATION_PRIORITY, + qt_params.LOCATION_MIN_INTERVAL, + qt_params.LOCATION_MAX_AGE, } if any(k in parameters for k in location_parameters): if all(k in parameters for k in location_parameters): - if parameters[constants.LOCATION_PRIORITY] not in { + if parameters[qt_params.LOCATION_PRIORITY] not in { "no-power", "low-power", "balanced", "high-accuracy", }: msg = ( - f"Parameter {constants.LOCATION_PRIORITY} must be set to " + f"Parameter {qt_params.LOCATION_PRIORITY.value} must be set to " "no-power, low-power, balanced, or high-accuracy:" - f"'{parameters[constants.LOCATION_PRIORITY]}' is an invalid value" + f"'{parameters[qt_params.LOCATION_PRIORITY]}' is an invalid value" ) raise PyXFormError(msg) try: - int(parameters[constants.LOCATION_MIN_INTERVAL]) + int(parameters[qt_params.LOCATION_MIN_INTERVAL]) except ValueError as lmi_err: raise PyXFormError( - "Parameter " - + constants.LOCATION_MIN_INTERVAL + f"Parameter {qt_params.LOCATION_MIN_INTERVAL.value}" + " must have an integer value." ) from lmi_err - if int(parameters[constants.LOCATION_MIN_INTERVAL]) < 0: + if int(parameters[qt_params.LOCATION_MIN_INTERVAL]) < 0: raise PyXFormError( - "Parameter " - + constants.LOCATION_MIN_INTERVAL + f"Parameter {qt_params.LOCATION_MIN_INTERVAL.value}" + " must be greater than or equal to zero." ) try: - int(parameters[constants.LOCATION_MAX_AGE]) + int(parameters[qt_params.LOCATION_MAX_AGE]) except ValueError as lma_err: raise PyXFormError( - "Parameter " - + constants.LOCATION_MAX_AGE + f"Parameter {qt_params.LOCATION_MAX_AGE.value}" + " must have an integer value." ) from lma_err - if int(parameters[constants.LOCATION_MAX_AGE]) < 0: + if int(parameters[qt_params.LOCATION_MAX_AGE]) < 0: raise PyXFormError( - "Parameter " - + constants.LOCATION_MAX_AGE + f"Parameter {qt_params.LOCATION_MAX_AGE.value}" + " must be greater than or equal to zero." ) - if int(parameters[constants.LOCATION_MAX_AGE]) < int( - parameters[constants.LOCATION_MIN_INTERVAL] + if int(parameters[qt_params.LOCATION_MAX_AGE]) < int( + parameters[qt_params.LOCATION_MIN_INTERVAL] ): raise PyXFormError( - "Parameter " - + constants.LOCATION_MAX_AGE - + " must be greater than or equal to " - + constants.LOCATION_MIN_INTERVAL - + "." + f"Parameter {qt_params.LOCATION_MAX_AGE.value}" + + f" must be greater than or equal to {qt_params.LOCATION_MIN_INTERVAL.value}." ) new_dict["bind"] = new_dict.get("bind", {}) new_dict["bind"].update( { - "odk:" + constants.LOCATION_MAX_AGE: parameters[ - constants.LOCATION_MAX_AGE + f"odk:{qt_params.LOCATION_MAX_AGE.value}": parameters[ + qt_params.LOCATION_MAX_AGE ], - "odk:" + constants.LOCATION_MIN_INTERVAL: parameters[ - constants.LOCATION_MIN_INTERVAL + f"odk:{qt_params.LOCATION_MIN_INTERVAL.value}": parameters[ + qt_params.LOCATION_MIN_INTERVAL.value ], - "odk:" + constants.LOCATION_PRIORITY: parameters[ - constants.LOCATION_PRIORITY + f"odk:{qt_params.LOCATION_PRIORITY.value}": parameters[ + qt_params.LOCATION_PRIORITY.value ], } ) else: raise PyXFormError( "To include location information in" - + " the audit, '" - + constants.LOCATION_PRIORITY - + "', '" - + constants.LOCATION_MIN_INTERVAL - + "', and '" - + constants.LOCATION_MAX_AGE - + "' are required" - + " parameters." + + f" the audit, '{qt_params.LOCATION_PRIORITY.value}'" + + f", '{qt_params.LOCATION_MIN_INTERVAL.value}'" + + f", and '{qt_params.LOCATION_MAX_AGE.value}'" + + " are required parameters." ) meta_children.append(new_dict) @@ -1033,54 +1018,54 @@ def workbook_to_json( new_json_dict = row.copy() new_json_dict[constants.TYPE] = select_type - select_params_allowed = constants.ParametersSelect if parse_dict["select_command"] in { "select_one_from_file", "select_multiple_from_file", }: - select_params_allowed = constants.ParametersSelectFromFile - - # Look at parameters column for select parameters - pv.validate( - parameters=parameters, - accepted=select_params_allowed, - row_number=row_number, - ) + qt_params = constants.ParametersSelectFromFile + pv.validate( + parameters=parameters, + accepted=qt_params, + row_number=row_number, + ) + if "value" in parameters: + select_from_file.value_or_label_check( + name="value", + value=parameters["value"], + row_number=row_number, + ) + if "label" in parameters: + select_from_file.value_or_label_check( + name="label", + value=parameters["label"], + row_number=row_number, + ) + else: + qt_params = constants.ParametersSelect + pv.validate( + parameters=parameters, + accepted=qt_params, + row_number=row_number, + ) - if "randomize" in parameters: - if ( - parameters["randomize"] != "true" - and parameters["randomize"] != "false" - ): + if qt_params.RANDOMIZE in parameters: + if parameters[qt_params.RANDOMIZE] not in {"true", "false"}: raise PyXFormError( - "randomize must be set to true or false: " - f"""'{parameters["randomize"]}' is an invalid value""" + f"{qt_params.RANDOMIZE.value} must be set to true or false: " + f"""'{parameters[qt_params.RANDOMIZE]}' is an invalid value""" ) - if "seed" in parameters: - if not is_pyxform_reference(parameters["seed"]): + if qt_params.SEED in parameters: + if not is_pyxform_reference(parameters[qt_params.SEED]): try: - float(parameters["seed"]) + float(parameters[qt_params.SEED]) except ValueError as seed_err: raise PyXFormError( - "seed value must be a number or a reference to another field." + f"{qt_params.SEED.value} value must be a number or a reference to another field." ) from seed_err - elif "seed" in parameters: + elif qt_params.SEED in parameters: raise PyXFormError( - "Parameters must include randomize=true to use a seed." - ) - - if "value" in parameters: - select_from_file.value_or_label_check( - name="value", - value=parameters["value"], - row_number=row_number, - ) - if "label" in parameters: - select_from_file.value_or_label_check( - name="label", - value=parameters["label"], - row_number=row_number, + f"Parameters must include {qt_params.RANDOMIZE.value}=true to use a {qt_params.SEED.value}." ) new_json_dict[constants.PARAMETERS] = parameters @@ -1154,7 +1139,7 @@ def workbook_to_json( # range question_type if question_type == "range": - tick_labelset = parameters.get("tick_labelset") + tick_labelset = parameters.get(constants.ParametersRange.TICK_LABELSET.value) new_dict = qt.process_range_question_type( row_number=row_number, @@ -1204,30 +1189,37 @@ def workbook_to_json( if row.get("default"): new_dict["default"] = process_image_default(row["default"]) + qt_params = constants.ParametersImage pv.validate( parameters=parameters, - accepted=constants.ParametersImage, + accepted=qt_params, row_number=row_number, ) - if "max-pixels" in parameters: + if qt_params.MAX_PIXELS in parameters: try: - int(parameters["max-pixels"]) + int(parameters[qt_params.MAX_PIXELS]) except ValueError as mp_err: raise PyXFormError( - "Parameter max-pixels must have an integer value." + f"Parameter {qt_params.MAX_PIXELS.value} must have an integer value." ) from mp_err new_dict["bind"] = new_dict.get("bind", {}) - new_dict["bind"].update({"orx:max-pixels": parameters["max-pixels"]}) + new_dict["bind"].update( + { + f"orx:{qt_params.MAX_PIXELS.value}": parameters[ + qt_params.MAX_PIXELS + ] + } + ) else: warnings.append( (ROW_FORMAT_STRING % row_number) - + " Use the max-pixels parameter to speed up submission sending and save storage space. Learn more: https://xlsform.org/#image" + + f" Use the {qt_params.MAX_PIXELS.value} parameter to speed up submission sending and save storage space. Learn more: https://xlsform.org/#image" ) - if "app" in parameters: + if qt_params.APP in parameters: if appearance is None or appearance == "annotate": - app_package_name = str(parameters["app"]) + app_package_name = str(parameters[qt_params.APP]) validation_result = validate_android_package_name(app_package_name) if validation_result is None: new_dict["control"] = new_dict.get("control", {}) @@ -1242,47 +1234,51 @@ def workbook_to_json( if question_type == "audio": new_dict = row.copy() + qt_params = constants.ParametersAudio pv.validate( parameters=parameters, - accepted=constants.ParametersAudio, + accepted=qt_params, row_number=row_number, ) - if "quality" in parameters: - if parameters["quality"] not in { + if qt_params.QUALITY in parameters: + if parameters[qt_params.QUALITY] not in { constants.AUDIO_QUALITY_VOICE_ONLY, constants.AUDIO_QUALITY_LOW, constants.AUDIO_QUALITY_NORMAL, constants.AUDIO_QUALITY_EXTERNAL, }: - raise PyXFormError("Invalid value for quality.") + raise PyXFormError(f"Invalid value for {qt_params.QUALITY.value}.") new_dict["bind"] = new_dict.get("bind", {}) - new_dict["bind"].update({"odk:quality": parameters["quality"]}) + new_dict["bind"].update( + {f"odk:{qt_params.QUALITY.value}": parameters[qt_params.QUALITY]} + ) parent_children_array.append(new_dict) continue if question_type == "background-audio": new_dict = row.copy() + qt_params = constants.ParametersAudio pv.validate( parameters=parameters, - accepted=constants.ParametersAudio, + accepted=qt_params, row_number=row_number, ) action = ( action_module.ActionLibrary.odk_recordaudio_instance_load.value.to_dict() ) - if "quality" in parameters: - if parameters["quality"] not in { + if qt_params.QUALITY in parameters: + if parameters[qt_params.QUALITY] not in { constants.AUDIO_QUALITY_VOICE_ONLY, constants.AUDIO_QUALITY_LOW, constants.AUDIO_QUALITY_NORMAL, }: - raise PyXFormError("Invalid value for quality.") + raise PyXFormError(f"Invalid value for {qt_params.QUALITY.value}.") - action["odk:quality"] = parameters["quality"] + action[f"odk:{qt_params.QUALITY.value}"] = parameters[qt_params.QUALITY] new_dict["actions"] = new_dict.get("actions", []) new_dict["actions"].append(action) @@ -1292,65 +1288,79 @@ def workbook_to_json( if question_type in {"geopoint", "geoshape", "geotrace"}: new_dict = row.copy() + new_dict["control"] = new_dict.get("control", {}) if question_type == "geopoint": + qt_params = constants.ParametersGeoPoint pv.validate( parameters=parameters, - accepted=constants.ParametersGeoPoint, + accepted=qt_params, row_number=row_number, ) + + if qt_params.CAPTURE_ACCURACY in parameters: + try: + float(parameters[qt_params.CAPTURE_ACCURACY]) + new_dict["control"].update( + {"accuracyThreshold": parameters[qt_params.CAPTURE_ACCURACY]} + ) + except ValueError as ca_err: + raise PyXFormError( + f"Parameter {qt_params.CAPTURE_ACCURACY.value} must have a numeric value" + ) from ca_err + + if qt_params.WARNING_ACCURACY in parameters: + try: + float(parameters[qt_params.WARNING_ACCURACY]) + new_dict["control"].update( + { + "unacceptableAccuracyThreshold": parameters[ + qt_params.WARNING_ACCURACY + ] + } + ) + except ValueError as wa_err: + raise PyXFormError( + f"Parameter {qt_params.WARNING_ACCURACY.value} must have a numeric value" + ) from wa_err + else: + qt_params = constants.ParametersGeo pv.validate( parameters=parameters, - accepted=constants.ParametersGeo, + accepted=qt_params, row_number=row_number, ) + if qt_params.INCREMENTAL in parameters: + try: + qt.validate_geo_parameter_incremental( + value=parameters[qt_params.INCREMENTAL] + ) + except PyXFormError as e: + e.context.update( + sheet=constants.SURVEY, + column=constants.PARAMETERS, + row=row_number, + ) + raise + else: + new_dict["control"][qt_params.INCREMENTAL.value] = "true" - if "allow-mock-accuracy" in parameters: - if parameters["allow-mock-accuracy"] not in {"true", "false"}: - raise PyXFormError("Invalid value for allow-mock-accuracy.") + if qt_params.ALLOW_MOCK_ACCURACY in parameters: + if parameters[qt_params.ALLOW_MOCK_ACCURACY] not in {"true", "false"}: + raise PyXFormError( + f"Invalid value for {qt_params.ALLOW_MOCK_ACCURACY.value}." + ) new_dict["bind"] = new_dict.get("bind", {}) new_dict["bind"].update( - {"odk:allow-mock-accuracy": parameters["allow-mock-accuracy"]} + { + f"odk:{qt_params.ALLOW_MOCK_ACCURACY.value}": parameters[ + qt_params.ALLOW_MOCK_ACCURACY + ] + } ) - new_dict["control"] = new_dict.get("control", {}) - if "capture-accuracy" in parameters: - try: - float(parameters["capture-accuracy"]) - new_dict["control"].update( - {"accuracyThreshold": parameters["capture-accuracy"]} - ) - except ValueError as ca_err: - raise PyXFormError( - "Parameter capture-accuracy must have a numeric value" - ) from ca_err - - if "warning-accuracy" in parameters: - try: - float(parameters["warning-accuracy"]) - new_dict["control"].update( - {"unacceptableAccuracyThreshold": parameters["warning-accuracy"]} - ) - except ValueError as wa_err: - raise PyXFormError( - "Parameter warning-accuracy must have a numeric value" - ) from wa_err - - if "incremental" in parameters: - try: - qt.validate_geo_parameter_incremental(value=parameters["incremental"]) - except PyXFormError as e: - e.context.update( - sheet=constants.SURVEY, - column=constants.PARAMETERS, - row=row_number, - ) - raise - else: - new_dict["control"]["incremental"] = "true" - parent_children_array.append(new_dict) continue # TODO: Consider adding some question_type validation here. From 9fdd143d3d0b85e7cfc9e00fae309d3430fbfc52 Mon Sep 17 00:00:00 2001 From: lindsay stevens Date: Fri, 12 Jun 2026 18:55:39 +1000 Subject: [PATCH 3/3] chg: tidy up remaining parameters referenced as strings, add test case - 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 --- pyxform/question.py | 8 ++++-- pyxform/validators/pyxform/question_types.py | 4 +-- pyxform/xls2json.py | 27 +++++++++++--------- tests/parsing/test_parameters.py | 1 + 4 files changed, 24 insertions(+), 16 deletions(-) diff --git a/pyxform/question.py b/pyxform/question.py index 8eb7f3c45..9e6187e37 100644 --- a/pyxform/question.py +++ b/pyxform/question.py @@ -401,8 +401,12 @@ def build_xml(self, survey: "Survey"): itemset_value_ref = DEFAULT_ITEMSET_VALUE_REF itemset_label_ref = DEFAULT_ITEMSET_LABEL_REF if self.parameters is not None: - itemset_value_ref = self.parameters.get("value", itemset_value_ref) - itemset_label_ref = self.parameters.get("label", itemset_label_ref) + itemset_value_ref = self.parameters.get( + constants.ParametersSelectFromFile.VALUE, itemset_value_ref + ) + itemset_label_ref = self.parameters.get( + constants.ParametersSelectFromFile.LABEL, itemset_label_ref + ) is_previous_question = has_pyxform_reference(self.itemset) diff --git a/pyxform/validators/pyxform/question_types.py b/pyxform/validators/pyxform/question_types.py index 1d80f4124..cf3596af2 100644 --- a/pyxform/validators/pyxform/question_types.py +++ b/pyxform/validators/pyxform/question_types.py @@ -143,7 +143,7 @@ def process_range_question_type( raise PyXFormError(ErrorCode.RANGE_008.value.format(row=row_number)) no_ticks_appearance = appearance and appearance == "no-ticks" - defaults = QUESTION_TYPE_DICT["range"]["parameters"] + defaults = QUESTION_TYPE_DICT["range"][co.PARAMETERS] # set defaults for key in defaults: if key not in parameters: @@ -276,6 +276,6 @@ def process_parameter(name: str) -> Decimal | None: row["bind"] = row.get("bind", {}) row["bind"].update({"type": "decimal"}) - row["parameters"] = parameters + row[co.PARAMETERS] = parameters return row diff --git a/pyxform/xls2json.py b/pyxform/xls2json.py index da5ca30e5..e5c591606 100644 --- a/pyxform/xls2json.py +++ b/pyxform/xls2json.py @@ -520,7 +520,7 @@ def workbook_to_json( ) parameters = parameters_parse( - raw_parameters=row.get("parameters", ""), + raw_parameters=row.get(constants.PARAMETERS, ""), row_number=row_number, ) @@ -1028,16 +1028,16 @@ def workbook_to_json( accepted=qt_params, row_number=row_number, ) - if "value" in parameters: + if qt_params.VALUE in parameters: select_from_file.value_or_label_check( - name="value", - value=parameters["value"], + name=qt_params.VALUE.value, + value=parameters[qt_params.VALUE], row_number=row_number, ) - if "label" in parameters: + if qt_params.LABEL in parameters: select_from_file.value_or_label_check( - name="label", - value=parameters["label"], + name=qt_params.LABEL.value, + value=parameters[qt_params.LABEL], row_number=row_number, ) else: @@ -1163,23 +1163,26 @@ def workbook_to_json( if question_type == "text": new_dict = row.copy() + qt_params = constants.ParametersText pv.validate( parameters=parameters, - accepted=constants.ParametersText, + accepted=qt_params, row_number=row_number, ) - if "rows" in parameters: + if qt_params.ROWS in parameters: try: - int(parameters["rows"]) + int(parameters[qt_params.ROWS]) except ValueError as rows_err: raise PyXFormError( (ROW_FORMAT_STRING % row_number) - + " Parameter rows must have an integer value." + + f" Parameter {qt_params.ROWS.value} must have an integer value." ) from rows_err new_dict["control"] = new_dict.get("control", {}) - new_dict["control"].update({"rows": parameters["rows"]}) + new_dict["control"].update( + {qt_params.ROWS.value: parameters[qt_params.ROWS]} + ) parent_children_array.append(new_dict) continue diff --git a/tests/parsing/test_parameters.py b/tests/parsing/test_parameters.py index 3e8453e19..87d6cfc6b 100644 --- a/tests/parsing/test_parameters.py +++ b/tests/parsing/test_parameters.py @@ -56,6 +56,7 @@ ("Missing equals sign and value", "value"), ("Missing key before equals", "=val"), ("Missing value after equals", "value="), + ("Whitespace only after equals", "value= "), ("Duplicate equals ", "value==val"), ("Key with no value assignment", "value=val label"), ("Spaces in values", "label=My Label"),