Skip to content

Conversation

aditkumar72
Copy link
Contributor

@aditkumar72 aditkumar72 commented Aug 22, 2024

Change Summary

Add support for annotated_types.Not

Related issue number

fix #10111

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @sydney-runkle

@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label Aug 22, 2024
Copy link

codspeed-hq bot commented Aug 22, 2024

CodSpeed Performance Report

Merging #10210 will not alter performance

Comparing aditkumar72:annotated-types-not (b2b8b69) with main (69ea94d)

Summary

✅ 17 untouched benchmarks

Copy link
Contributor

github-actions bot commented Aug 22, 2024

Coverage report

This PR does not seem to contain any modification to coverable code.

@aditkumar72
Copy link
Contributor Author

please review

Copy link
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, nice work on the tests.

I left some change requests regarding the implementation of the constraint.

Comment on lines 329 to 347
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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...

Copy link
Contributor Author

@aditkumar72 aditkumar72 Aug 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I'm inclined to just support callables, as the signature indicates - feel free to modify the tests accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if things could be adapted on the annotated-types side. Not(<constraint>) looks like a useful pattern

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Zac-HD, thoughts ^^?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sketched out an idea in annotated-types/annotated-types#74 (review) but nobody's volunteered to implement it yet 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aditkumar72 let's implement support in pydantic given the existing api for Not, then we can expand it down the line.

Comment on lines 315 to 328
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the not in the val_func, this is basically identical to the Predicate logic - can we consolidate this logic?

@pydantic-hooky pydantic-hooky bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Aug 22, 2024
Copy link
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slow, but seems fine for now (all of these annotation applications are, needs a general rework as mentioned in other locations as well).

@sydney-runkle sydney-runkle enabled auto-merge (squash) August 26, 2024 14:48
@sydney-runkle sydney-runkle merged commit ae476e3 into pydantic:main Aug 26, 2024
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting author revision awaiting changes from the PR author relnotes-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for annotated_types.Not
4 participants