Validation tests #20
Validation tests #20
Conversation
mrm9084
requested review from
Eskibear,
albertofori,
amerjusupovic,
avanigupta and
rossgrambo
| def _validate(self): | ||
| if not isinstance(self._id, str): | ||
| raise ValueError("Feature flag id field must be a string.") | ||
| raise ValueError(f"Invalid setting 'id' with value '{self._id}' for feature '{self._id}'.") |
There was a problem hiding this comment.
If the class of this instance does not have a str or repr defined, the value could appear weird in the error message. e.g. '<__main__.SomeClass object at 0x000001B988224A60>'
How about something along these lines:
Invalid type for setting 'id'. 'str' expected but {type(self._id).__name__} given
There was a problem hiding this comment.
This change was to start moving us towards consistent error messages between the libraries, this is the exact way DotNet does the message.
There was a problem hiding this comment.
I like the idea of being consistent, but in dotnet- I don't think this error will ever happen on name/id and I think the wording is weird here for the str check.
This error will only happen if i'ts not a string, but when logging it would say something like:
Invalid setting 'id' with value 'null' for feature 'null' - which is kind of clear, but number and object could be weird: Invalid setting 'id' with value '1' for feature '1', because string:"1" is a valid name but number:1 is not.
I prefer something closer to @albertofori 's suggestion.
There was a problem hiding this comment.
This isn't a great example as 1 is valid as python is typeless.
I can update this in a sepreate PR, I can change to what Albert mentioned, it's the change bellow this one that is 1 to 1 with Dot Net.
| def _validate(self): | ||
| if not isinstance(self._id, str): | ||
| raise ValueError("Feature flag id field must be a string.") | ||
| raise ValueError(f"Invalid setting 'id' with value '{self._id}' for feature '{self._id}'.") |
There was a problem hiding this comment.
I like the idea of being consistent, but in dotnet- I don't think this error will ever happen on name/id and I think the wording is weird here for the str check.
This error will only happen if i'ts not a string, but when logging it would say something like:
Invalid setting 'id' with value 'null' for feature 'null' - which is kind of clear, but number and object could be weird: Invalid setting 'id' with value '1' for feature '1', because string:"1" is a valid name but number:1 is not.
I prefer something closer to @albertofori 's suggestion.
Description
Updates the validation tests to be across multiple files.