Improve error message in the QB #3682
Conversation
Previous message was not very helpful.
There was a problem hiding this comment.
Thanks @ramirezfranciscof . Two small suggested improvements and regarding the commit message, maybe we can make it a bit more informative. How about:
`QueryBuilder`: fix validation bug and improve message for `in` operator
The order of validation checks was incorrect and the type was not
checked at all. In addition the validation error message clarity has
been improved to contain the operator name.
| @@ -206,11 +206,11 @@ def get_filter_expr(self, operator, value, attr_key, is_attribute, alias=None, c | |||
| ''.format(operator, value) | |||
| ) | |||
| elif operator == 'in': | |||
| if not value: | |||
There was a problem hiding this comment.
what if value is not even an interable? This will raise a TypeError, so maybe it is better to first check that a list/tuple is passed.
There was a problem hiding this comment.
Change implemented. In the end I ended up using a try/except statement instead of checking for explicit types (value could also be a set maybe?) because apparently "checking for iterables" is kind of controversial and this is a more "pythonic" approach (sauce). Let me know if you still prefer explicit type checking.
There was a problem hiding this comment.
Normally that depends on your interface whether it declares a specific type, but in this case I don't think we do so I like your solution 👍
| @@ -206,11 +206,11 @@ def get_filter_expr(self, operator, value, attr_key, is_attribute, alias=None, c | |||
| ''.format(operator, value) | |||
| ) | |||
| elif operator == 'in': | |||
| if not value: | |||
| raise InputValidationError('Value for operator \'in\' is an empty list') | |||
There was a problem hiding this comment.
We indeed prefer single quotes for string literals but not when that requires escaping internal quotes. In this case favor
'Value for operator `in` is an empty list'
or double quotes
'Value for operator "in" is an empty list'
or if you really want single quotes
"Value for operator 'in' is an empty list"
Changed again the order of operations to add a first safeguard that checks if all values provided are effectively iterables. Doing this before the 'if not' clause also catches any 'None' values that could otherwise trigger an 'empty list' error message.
Previous message was not very helpful.