refactor(api): more clear error messages for type validation when creating runtime parameters #14903
Conversation
| @@ -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." | |||
There was a problem hiding this comment.
Probably at most rather than less than if the maximum is inclusive.
| ) | ||
| if len(unit) > UNIT_MAX_LEN: | ||
| raise ParameterNameError( | ||
| f"Description {unit} greater than {UNIT_MAX_LEN} characters." |
There was a problem hiding this comment.
| f"Description {unit} greater than {UNIT_MAX_LEN} characters." | |
| f"Unit {unit} greater than {UNIT_MAX_LEN} characters." |
| @@ -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}" | |||
There was a problem hiding this comment.
match or be of (or something else) — what's most Pythonic, or most natural to protocol authors?
| raise ParameterDefinitionError( | ||
| "Maximum must be greater than the minimum" | ||
| f"Minimum is type {type(minimum)}, must match parameter type {parameter_type}" |
There was a problem hiding this comment.
comma splice, easy to fix by adding a conjunction
| 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.
| ) | ||
| 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." |
There was a problem hiding this comment.
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.
| 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}." |
There was a problem hiding this comment.
apply two decisions from above:
is of type(or other term) instead ofhas type- add
butto resolve comma splice
| ) | ||
| if len(description) > DESCRIPTION_MAX_LEN: | ||
| raise ParameterNameError( | ||
| f"Description {description} greater than {DESCRIPTION_MAX_LEN} characters." |
There was a problem hiding this comment.
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.
| @@ -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}" | |||
There was a problem hiding this comment.
Maybe write it as f".. type '{parameter_type}'." (with quotes) to match the formatting of internal python type errors?
| raise ParameterDefinitionError( | ||
| f"Minimum and maximum must match type {parameter_type}" | ||
| f"Maximum is type {type(minimum)}, must match parameter type {parameter_type}" |
There was a problem hiding this comment.
| 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: |
…ating runtime parameters (#14903)
Overview
This PR closes a bunch of tickets related to unclear/confusing/incorrect error messages in failed analysis for RTP protocols.
The first set of fixes applies to AUTH-303, AUTH-305, AUTH-307 and AUTH-309, which all relate to string validation for
variable_name,display_name(for both the parameter and choices) andunit. For these, we were not explicitly checking that the value given was a string, which could lead to related but unclear messages when those types inevitably would fail at some step on the process. Now, before we do any validation on string length, we ensure the value given is a string and raise an appropriate error message if it is not.AUTH-304 related to an unclear message if the
valuekey in a parameter choice was of the wrong type. This was a case of accidentally wrappingtypearound a type object, causing it to print<class 'type'>instead of<class 'str'>for example. This was a simple fix of just removing thetype()call.AUTH-306 was about unclear messages for the wrong type being used for minimum and maximum. The validation steps used for those was shifted around to more helpfully catch and point to incorrect types, rather than more generic error messages that didn't say which value was wrong.
Finally, AUTH-308 similarly brought up that there was no distinct error message when the
defaultvalue was of the wrong type. To fix for this was just to make a bespoke check for that to raise a unique error message, rather than using the same check that setting thevalueuses. Its a small instance of repeated code but the check is simple enough and unlikely to be changed.Test Plan
Ran through all the test protocols linked in the tickets above and ensured that they all had clear error messages relating to each individual one.
Changelog
valuenot printing out the actual type that's expectedminimum/maximumvalidation to provide more clear error text to disambiguate which value is wrong.defaultvalue is of the wrong type.Review requests
Risk assessment
Low