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 1 commit
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
61 changes: 42 additions & 19 deletions api/src/opentrons/protocols/parameters/validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably at most rather than less than if the maximum is inclusive.

)
if len(display_name) > DISPLAY_NAME_MAX_LEN:
raise ParameterNameError(
f"Display name {display_name} greater than {DISPLAY_NAME_MAX_LEN} characters."
Expand All @@ -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."
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 less than {DESCRIPTION_MAX_LEN} characters."
)
if len(description) > DESCRIPTION_MAX_LEN:
raise ParameterNameError(
f"Description {description} greater than {DESCRIPTION_MAX_LEN} characters."
Copy link
Contributor

Choose a reason for hiding this comment

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

Does {description} need to be enclosed in quotes so it doesn't run into the rest of the error text? It can be arbitrary text without punctuation, so it might be hard to read otherwise.

)
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."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
f"Description {unit} greater than {UNIT_MAX_LEN} characters."
f"Unit {unit} greater than {UNIT_MAX_LEN} characters."

)
return unit


Expand Down Expand Up @@ -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}"
Copy link
Contributor

Choose a reason for hiding this comment

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

match or be of (or something else) — what's most Pythonic, or most natural to protocol authors?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe write it as f".. type '{parameter_type}'." (with quotes) to match the formatting of internal python type errors?

)


Expand All @@ -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}"
Copy link
Contributor

Choose a reason for hiding this comment

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

comma splice, easy to fix by adding a conjunction

Suggested change
f"Minimum is type {type(minimum)}, must match parameter type {parameter_type}"
f"Minimum is type {type(minimum)}, but must match parameter type {parameter_type}"

replace match here if changed above.

)

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}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
f"Maximum is type {type(minimum)}, must match parameter type {parameter_type}"
f"Maximum is type {type(maximum)}, 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:
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"
f"Parameter of type {parameter_type} does not support minimum or maximum arguments."
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the previous revision of this message was more informative. It says what does support min/max, not only that the current type doesn't. And I can't see a future type that isn't int or float supporting them, so no risk there.

)


Expand All @@ -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}."
Copy link
Contributor

Choose a reason for hiding this comment

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

apply two decisions from above:

  • is of type (or other term) instead of has type
  • add but to resolve comma splice

)

if choices is None and minimum is None and maximum is None:
raise ParameterDefinitionError(
Expand Down
7 changes: 4 additions & 3 deletions api/tests/opentrons/protocols/parameters/test_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Loading