From 96459734752c4e7ea651e62e914b586e6ac3db05 Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Mon, 15 Apr 2024 13:05:47 -0400 Subject: [PATCH 1/4] more clear/distinct error messages --- .../protocols/parameters/validation.py | 61 +++++++++++++------ .../protocols/parameters/test_validation.py | 7 ++- 2 files changed, 46 insertions(+), 22 deletions(-) diff --git a/api/src/opentrons/protocols/parameters/validation.py b/api/src/opentrons/protocols/parameters/validation.py index 9410db294ed..20d8c55dea8 100644 --- a/api/src/opentrons/protocols/parameters/validation.py +++ b/api/src/opentrons/protocols/parameters/validation.py @@ -29,6 +29,10 @@ def validate_variable_name_unique( def ensure_display_name(display_name: str) -> str: """Validate display name is within the character limit.""" + if not isinstance(display_name, str): + raise ParameterNameError( + f"Display name must be a string and less than {DISPLAY_NAME_MAX_LEN} characters." + ) if len(display_name) > DISPLAY_NAME_MAX_LEN: raise ParameterNameError( f"Display name {display_name} greater than {DISPLAY_NAME_MAX_LEN} characters." @@ -38,6 +42,8 @@ def ensure_display_name(display_name: str) -> str: def ensure_variable_name(variable_name: str) -> str: """Validate variable name is a valid python variable name.""" + if not isinstance(variable_name, str): + raise ParameterNameError("Variable name must be a string.") if not variable_name.isidentifier(): raise ParameterNameError( "Variable name must only contain alphanumeric characters, underscores, and cannot start with a digit." @@ -49,19 +55,29 @@ def ensure_variable_name(variable_name: str) -> str: def ensure_description(description: Optional[str]) -> Optional[str]: """Validate description is within the character limit.""" - if description is not None and len(description) > DESCRIPTION_MAX_LEN: - raise ParameterNameError( - f"Description {description} greater than {DESCRIPTION_MAX_LEN} characters." - ) + if description is not None: + if not isinstance(description, str): + raise ParameterNameError( + f"Description must be a string and less than {DESCRIPTION_MAX_LEN} characters." + ) + if len(description) > DESCRIPTION_MAX_LEN: + raise ParameterNameError( + f"Description {description} greater than {DESCRIPTION_MAX_LEN} characters." + ) return description def ensure_unit_string_length(unit: Optional[str]) -> Optional[str]: """Validate unit is within the character limit.""" - if unit is not None and len(unit) > UNIT_MAX_LEN: - raise ParameterNameError( - f"Description {unit} greater than {UNIT_MAX_LEN} characters." - ) + if unit is not None: + if not isinstance(unit, str): + raise ParameterNameError( + f"Unit must be a string and less than {UNIT_MAX_LEN} characters." + ) + if len(unit) > UNIT_MAX_LEN: + raise ParameterNameError( + f"Description {unit} greater than {UNIT_MAX_LEN} characters." + ) return unit @@ -173,7 +189,7 @@ def _validate_choices( ensure_display_name(display_name) if not isinstance(value, parameter_type): raise ParameterDefinitionError( - f"All choices provided must match type {type(parameter_type)}" + f"All choices provided must match type {parameter_type}" ) @@ -192,21 +208,25 @@ def _validate_min_and_max( "If a maximum value is provided a minimum must also be provided." ) elif maximum is not None and minimum is not None: - if isinstance(maximum, (int, float)) and isinstance(minimum, (int, float)): - if maximum <= minimum: + if parameter_type is int or parameter_type is float: + if not isinstance(minimum, parameter_type): raise ParameterDefinitionError( - "Maximum must be greater than the minimum" + f"Minimum is type {type(minimum)}, must match parameter type {parameter_type}" ) - - if not isinstance(minimum, parameter_type) or not isinstance( - maximum, parameter_type - ): + if not isinstance(maximum, parameter_type): raise ParameterDefinitionError( - f"Minimum and maximum must match type {parameter_type}" + f"Maximum is type {type(minimum)}, must match parameter type {parameter_type}" + ) + # These asserts are for the type checker and should never actually be asserted false + assert isinstance(minimum, (int, float)) + assert isinstance(maximum, (int, float)) + if maximum <= minimum: + raise ParameterDefinitionError( + "Maximum must be greater than the minimum" ) else: raise ParameterDefinitionError( - "Only parameters of type float or int can have a minimum and maximum" + f"Parameter of type {parameter_type} does not support minimum or maximum arguments." ) @@ -226,7 +246,10 @@ def validate_options( parameter_type: type, ) -> None: """Validate default values and all possible constraints for a valid parameter definition.""" - validate_type(default, parameter_type) + if not isinstance(default, parameter_type): + raise ParameterValueError( + f"Parameter default {default} has type {type(default)}, must match type {parameter_type}." + ) if choices is None and minimum is None and maximum is None: raise ParameterDefinitionError( diff --git a/api/tests/opentrons/protocols/parameters/test_validation.py b/api/tests/opentrons/protocols/parameters/test_validation.py index 4d3b2fc83b5..39c6a27563f 100644 --- a/api/tests/opentrons/protocols/parameters/test_validation.py +++ b/api/tests/opentrons/protocols/parameters/test_validation.py @@ -283,9 +283,10 @@ def test_convert_type_string_for_num_param_raises(param_type: type) -> None: (123, 1, None, None, int, "maximum must also"), (123, None, 100, None, int, "minimum must also"), (123, 100, 1, None, int, "Maximum must be greater"), - (123, 1.1, 100, None, int, "Minimum and maximum must match type"), - (123, 1, 100.5, None, int, "Minimum and maximum must match type"), - (123, "1", "100", None, int, "Only parameters of type float or int"), + (123, 1.1, 100, None, int, "Minimum is type"), + (123, 1, 100.5, None, int, "Maximum is type"), + (123.0, "1.0", 100.0, None, float, "Minimum is type"), + ("blah", 1, 100, None, str, "does not support minimum or maximum"), ], ) def test_validate_options_raise_definition_error( From 5999e34ced77607d354712332526f1166d3e117e Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Mon, 15 Apr 2024 15:22:04 -0400 Subject: [PATCH 2/4] tweaks to wording/punctuation --- .../protocols/parameters/validation.py | 24 +++++++++---------- .../protocols/parameters/test_validation.py | 4 ++-- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/api/src/opentrons/protocols/parameters/validation.py b/api/src/opentrons/protocols/parameters/validation.py index 20d8c55dea8..d8c3fa6ae0a 100644 --- a/api/src/opentrons/protocols/parameters/validation.py +++ b/api/src/opentrons/protocols/parameters/validation.py @@ -31,11 +31,11 @@ def ensure_display_name(display_name: str) -> str: """Validate display name is within the character limit.""" if not isinstance(display_name, str): raise ParameterNameError( - f"Display name must be a string and less than {DISPLAY_NAME_MAX_LEN} characters." + f"Display name must be a string and at most {DISPLAY_NAME_MAX_LEN} characters." ) if len(display_name) > DISPLAY_NAME_MAX_LEN: raise ParameterNameError( - f"Display name {display_name} greater than {DISPLAY_NAME_MAX_LEN} characters." + f'Display name "{display_name}" greater than {DISPLAY_NAME_MAX_LEN} characters.' ) return display_name @@ -58,11 +58,11 @@ def ensure_description(description: Optional[str]) -> Optional[str]: if description is not None: if not isinstance(description, str): raise ParameterNameError( - f"Description must be a string and less than {DESCRIPTION_MAX_LEN} characters." + f"Description must be a string and at most {DESCRIPTION_MAX_LEN} characters." ) if len(description) > DESCRIPTION_MAX_LEN: raise ParameterNameError( - f"Description {description} greater than {DESCRIPTION_MAX_LEN} characters." + f'Description "{description}" greater than {DESCRIPTION_MAX_LEN} characters.' ) return description @@ -72,11 +72,11 @@ def ensure_unit_string_length(unit: Optional[str]) -> Optional[str]: if unit is not None: if not isinstance(unit, str): raise ParameterNameError( - f"Unit must be a string and less than {UNIT_MAX_LEN} characters." + f"Unit must be a string and at most {UNIT_MAX_LEN} characters." ) if len(unit) > UNIT_MAX_LEN: raise ParameterNameError( - f"Description {unit} greater than {UNIT_MAX_LEN} characters." + f'Unit "{unit}" greater than {UNIT_MAX_LEN} characters.' ) return unit @@ -189,7 +189,7 @@ def _validate_choices( ensure_display_name(display_name) if not isinstance(value, parameter_type): raise ParameterDefinitionError( - f"All choices provided must match type {parameter_type}" + f"All choices provided must be of type {parameter_type}" ) @@ -211,11 +211,11 @@ def _validate_min_and_max( if parameter_type is int or parameter_type is float: if not isinstance(minimum, parameter_type): raise ParameterDefinitionError( - f"Minimum is type {type(minimum)}, must match parameter type {parameter_type}" + f"Minimum is type {type(minimum)}, but must be of parameter type {parameter_type}" ) if not isinstance(maximum, parameter_type): raise ParameterDefinitionError( - f"Maximum is type {type(minimum)}, must match parameter type {parameter_type}" + f"Maximum is type {type(maximum)}, but must be of parameter type {parameter_type}" ) # These asserts are for the type checker and should never actually be asserted false assert isinstance(minimum, (int, float)) @@ -226,7 +226,7 @@ def _validate_min_and_max( ) else: raise ParameterDefinitionError( - f"Parameter of type {parameter_type} does not support minimum or maximum arguments." + "Only parameters of type float or int can have a minimum and maximum." ) @@ -234,7 +234,7 @@ def validate_type(value: ParamType, parameter_type: type) -> None: """Validate parameter value is the correct type.""" if not isinstance(value, parameter_type): raise ParameterValueError( - f"Parameter value {value} has type {type(value)}, must match type {parameter_type}." + f"Parameter value {value} has type {type(value)}, but must be of type {parameter_type}." ) @@ -248,7 +248,7 @@ def validate_options( """Validate default values and all possible constraints for a valid parameter definition.""" if not isinstance(default, parameter_type): raise ParameterValueError( - f"Parameter default {default} has type {type(default)}, must match type {parameter_type}." + f"Parameter default {default} has type {type(default)}, but must be of type {parameter_type}." ) if choices is None and minimum is None and maximum is None: diff --git a/api/tests/opentrons/protocols/parameters/test_validation.py b/api/tests/opentrons/protocols/parameters/test_validation.py index 39c6a27563f..4206d3d3cd4 100644 --- a/api/tests/opentrons/protocols/parameters/test_validation.py +++ b/api/tests/opentrons/protocols/parameters/test_validation.py @@ -278,7 +278,7 @@ def test_convert_type_string_for_num_param_raises(param_type: type) -> None: None, [{"display_name": "abc", "value": "123"}], int, - "must match type", + "must be of type", ), (123, 1, None, None, int, "maximum must also"), (123, None, 100, None, int, "minimum must also"), @@ -286,7 +286,7 @@ def test_convert_type_string_for_num_param_raises(param_type: type) -> None: (123, 1.1, 100, None, int, "Minimum is type"), (123, 1, 100.5, None, int, "Maximum is type"), (123.0, "1.0", 100.0, None, float, "Minimum is type"), - ("blah", 1, 100, None, str, "does not support minimum or maximum"), + ("blah", 1, 100, None, str, "Only parameters of type float or int"), ], ) def test_validate_options_raise_definition_error( From 55b6c19da765bf4ac46d14db210253ee8d56af83 Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Mon, 15 Apr 2024 15:31:37 -0400 Subject: [PATCH 3/4] format types like 'str' instead of --- .../protocols/parameters/validation.py | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/api/src/opentrons/protocols/parameters/validation.py b/api/src/opentrons/protocols/parameters/validation.py index d8c3fa6ae0a..adde67b0481 100644 --- a/api/src/opentrons/protocols/parameters/validation.py +++ b/api/src/opentrons/protocols/parameters/validation.py @@ -151,7 +151,7 @@ def convert_type_string_for_enum( return "str" else: raise ParameterValueError( - f"Cannot resolve parameter type {parameter_type} for an enumerated parameter." + f"Cannot resolve parameter type '{parameter_type.__name__}' for an enumerated parameter." ) @@ -163,7 +163,7 @@ def convert_type_string_for_num_param(parameter_type: type) -> Literal["int", "f return "float" else: raise ParameterValueError( - f"Cannot resolve parameter type {parameter_type} for a number parameter." + f"Cannot resolve parameter type '{parameter_type.__name__}' for a number parameter." ) @@ -189,7 +189,7 @@ def _validate_choices( ensure_display_name(display_name) if not isinstance(value, parameter_type): raise ParameterDefinitionError( - f"All choices provided must be of type {parameter_type}" + f"All choices provided must be of type '{parameter_type.__name__}'" ) @@ -211,11 +211,13 @@ def _validate_min_and_max( if parameter_type is int or parameter_type is float: if not isinstance(minimum, parameter_type): raise ParameterDefinitionError( - f"Minimum is type {type(minimum)}, but must be of parameter type {parameter_type}" + f"Minimum is type '{type(minimum).__name__}'," + f" but must be of parameter type '{parameter_type.__name__}'" ) if not isinstance(maximum, parameter_type): raise ParameterDefinitionError( - f"Maximum is type {type(maximum)}, but must be of parameter type {parameter_type}" + f"Maximum is type {type(maximum).__name__}," + f" but must be of parameter type '{parameter_type.__name__}'" ) # These asserts are for the type checker and should never actually be asserted false assert isinstance(minimum, (int, float)) @@ -234,7 +236,8 @@ def validate_type(value: ParamType, parameter_type: type) -> None: """Validate parameter value is the correct type.""" if not isinstance(value, parameter_type): raise ParameterValueError( - f"Parameter value {value} has type {type(value)}, but must be of type {parameter_type}." + f"Parameter value {value} has type '{type(value).__name__}'," + f" but must be of type '{parameter_type.__name__}'." ) @@ -248,7 +251,8 @@ def validate_options( """Validate default values and all possible constraints for a valid parameter definition.""" if not isinstance(default, parameter_type): raise ParameterValueError( - f"Parameter default {default} has type {type(default)}, but must be of type {parameter_type}." + f"Parameter default {default} has type '{type(default).__name__}'," + f" but must be of type '{parameter_type.__name__}'." ) if choices is None and minimum is None and maximum is None: From 8b9a16586a5f36a6d8629a870b040ac32010b46e Mon Sep 17 00:00:00 2001 From: jbleon95 Date: Mon, 15 Apr 2024 16:05:37 -0400 Subject: [PATCH 4/4] missing single quotes for a printed type --- api/src/opentrons/protocols/parameters/validation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/opentrons/protocols/parameters/validation.py b/api/src/opentrons/protocols/parameters/validation.py index adde67b0481..2db343c71b6 100644 --- a/api/src/opentrons/protocols/parameters/validation.py +++ b/api/src/opentrons/protocols/parameters/validation.py @@ -216,7 +216,7 @@ def _validate_min_and_max( ) if not isinstance(maximum, parameter_type): raise ParameterDefinitionError( - f"Maximum is type {type(maximum).__name__}," + f"Maximum is type '{type(maximum).__name__}'," f" but must be of parameter type '{parameter_type.__name__}'" ) # These asserts are for the type checker and should never actually be asserted false