Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(api): more clear error messages for type validation when creating runtime parameters #14903

Merged
merged 4 commits into from
Apr 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 50 additions & 23 deletions api/src/opentrons/protocols/parameters/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,21 @@ 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 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


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."
Expand All @@ -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 at most {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 at most {UNIT_MAX_LEN} characters."
)
if len(unit) > UNIT_MAX_LEN:
raise ParameterNameError(
f'Unit "{unit}" greater than {UNIT_MAX_LEN} characters.'
)
return unit


Expand Down Expand Up @@ -135,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."
)


Expand All @@ -147,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."
)


Expand All @@ -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 be of type '{parameter_type.__name__}'"
)


Expand All @@ -192,29 +208,36 @@ 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).__name__}',"
f" but must be of parameter type '{parameter_type.__name__}'"
)

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(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))
assert isinstance(maximum, (int, float))
if maximum <= minimum:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't notice this before and probably should check with @ecormany @y3rsh ..should we allow 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"
"Only parameters of type float or int can have a minimum and maximum."
)


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).__name__}',"
f" but must be of type '{parameter_type.__name__}'."
)


Expand All @@ -226,7 +249,11 @@ 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).__name__}',"
f" but must be of type '{parameter_type.__name__}'."
)

if choices is None and minimum is None and maximum is None:
raise ParameterDefinitionError(
Expand Down
9 changes: 5 additions & 4 deletions api/tests/opentrons/protocols/parameters/test_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -278,14 +278,15 @@ 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"),
(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, "Only parameters of type float or int"),
],
)
def test_validate_options_raise_definition_error(
Expand Down
Loading