Add support for annotated_types.Not#10210
Conversation
CodSpeed Performance ReportMerging #10210 will not alter performanceComparing Summary
|
Coverage report
This PR does not seem to contain any modification to coverable code. |
f364c27 to
c7013f7
Compare
|
please review |
sydney-runkle
left a comment
There was a problem hiding this comment.
Looking good, nice work on the tests.
I left some change requests regarding the implementation of the constraint.
| elif hasattr(annotation.func, '__class__'): | ||
| class_name = annotation.func.__class__.__qualname__ | ||
|
|
||
| if (annotation_type := type(annotation.func)) in (at_to_constraint_map := _get_at_to_constraint_map()): | ||
| constraint = at_to_constraint_map[annotation_type] | ||
|
|
||
| def val_func(v: Any) -> Any: | ||
| try: | ||
| if get_constraint_validator(constraint)(v, getattr(annotation.func, constraint)) is v: # noqa: B023 | ||
| raise PydanticCustomError( | ||
| 'not_operation_failed', | ||
| f'Not of {class_name} failed', # type: ignore # noqa: B023 | ||
| ) | ||
| except PydanticKnownError: | ||
| pass | ||
|
|
||
| return v | ||
|
|
||
| schema = cs.no_info_after_validator_function(val_func, schema) |
There was a problem hiding this comment.
| elif hasattr(annotation.func, '__class__'): | |
| class_name = annotation.func.__class__.__qualname__ | |
| if (annotation_type := type(annotation.func)) in (at_to_constraint_map := _get_at_to_constraint_map()): | |
| constraint = at_to_constraint_map[annotation_type] | |
| def val_func(v: Any) -> Any: | |
| try: | |
| if get_constraint_validator(constraint)(v, getattr(annotation.func, constraint)) is v: # noqa: B023 | |
| raise PydanticCustomError( | |
| 'not_operation_failed', | |
| f'Not of {class_name} failed', # type: ignore # noqa: B023 | |
| ) | |
| except PydanticKnownError: | |
| pass | |
| return v | |
| schema = cs.no_info_after_validator_function(val_func, schema) |
I don't think we need this logic...
There was a problem hiding this comment.
@sydney-runkle
please correct me If I am wrong, but removing this logic will not support:
annotated_types.Not(annotated_types.MultipleOf(2)) or any other annotated types like Ge, Le, etc.. with Not
There was a problem hiding this comment.
@aditkumar72, that's true, but the signature of Not indicates that it takes: func: Callable[[Any], bool], so I don't feel like we should support types like at.Not(Gt(2)) because that's not type safe:
Argument of type "Gt" cannot be assigned to parameter "func" of type "(Any) -> bool" in function "__init__"
Type "Gt" is incompatible with type "(Any) -> bool"
There was a problem hiding this comment.
So I'm inclined to just support callables, as the signature indicates - feel free to modify the tests accordingly.
There was a problem hiding this comment.
I realize this is contradictory to the initial feature request - I'd suggest that folks can design a custom Not type if they'd like to go that route.
There was a problem hiding this comment.
I wonder if things could be adapted on the annotated-types side. Not(<constraint>) looks like a useful pattern
There was a problem hiding this comment.
I sketched out an idea in annotated-types/annotated-types#74 (review) but nobody's volunteered to implement it yet 🙂
There was a problem hiding this comment.
@aditkumar72 let's implement support in pydantic given the existing api for Not, then we can expand it down the line.
| elif isinstance(annotation, at.Not): | ||
| if hasattr(annotation.func, '__qualname__'): | ||
| func_name = annotation.func.__qualname__ | ||
|
|
||
| def val_func(v: Any) -> Any: | ||
| if annotation.func(v): # noqa: B023 | ||
| raise PydanticCustomError( | ||
| 'not_operation_failed', | ||
| f'Not of {func_name} failed', # type: ignore # noqa: B023 | ||
| ) | ||
|
|
||
| return v | ||
|
|
||
| schema = cs.no_info_after_validator_function(val_func, schema) |
There was a problem hiding this comment.
Other than the not in the val_func, this is basically identical to the Predicate logic - can we consolidate this logic?
sydney-runkle
left a comment
There was a problem hiding this comment.
Slow, but seems fine for now (all of these annotation applications are, needs a general rework as mentioned in other locations as well).
Change Summary
Add support for annotated_types.Not
Related issue number
fix #10111
Checklist
Selected Reviewer: @sydney-runkle