Fix equality checks for primitives in literals #1459
Conversation
CodSpeed Performance ReportMerging #1459 will not alter performanceComparing Summary
|
src/validators/literal.rs
Outdated
| expected_str: (!expected_str.is_empty()).then_some(expected_str), | ||
| expected_py_dict: (!expected_py_dict.is_empty()).then_some(expected_py_dict.into()), | ||
| expected_py_values: (!expected_py_values.is_empty()).then_some(expected_py_values), | ||
| expected_py_primitives: (!expected_py_primitives.is_empty()).then_some(expected_py_primitives), |
There was a problem hiding this comment.
I wonder, is there an advantage of expected_py_primitives over expected_py_dict?
There was a problem hiding this comment.
Conclusion was that expected_py_primitives will be used only in lax mode. I think it probably makes sense to store as a dict and use hash lookup (as that checks __hash__ and __eq__) - we can always make more lax later.
There was a problem hiding this comment.
Also conclusion - we're not going to fall back to union validation, that's too lax + time consuming.
There was a problem hiding this comment.
Alright - can't enforce strict / lax here as we're in the literal lookup, but I've added the hash check.
There was a problem hiding this comment.
If enforcing the hash check, surely a dict is just straight up more performant?
There was a problem hiding this comment.
Overall I'm ok with this, I think, though we should check we all agree that making Literal validation a little more lax (via __eq__) is desirable.
FWIW I think the alternative we understood to be to fall back to union validation if the input didn't validate successfuly.
Fix pydantic/pydantic#9989
Also just some minor tidying of the surrounding code.
Maybe we don't want this change - this could be a bit dangerous with custom types that implement
__eq__. I'm not opposed to closing the above issue and closing this PR, but this at least presents a solution if we do want to move forward.