Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updates type signature for Field() constructor #9484

Merged
merged 2 commits into from
May 29, 2024

Conversation

bjmc
Copy link
Contributor

@bjmc bjmc commented May 22, 2024

Replaces float in lt/gt/le/ge, arguments with the appropriate Protocol from annotated_types.

Change Summary

Related to #9472 updates the type signatures in Field() to be less restrictive, using the appropriate protocols from annotated_types instead of strict float or int types. This should be a nonbreaking change.

Related issue number

fixes #9472

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

@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label May 22, 2024
Replaces 'float' or 'int' in lt/gt/le/ge,
multiple_of, min/max_length arguments with the
appropriate Protocol from annotated_types.
Copy link

codspeed-hq bot commented May 22, 2024

CodSpeed Performance Report

Merging #9484 will not alter performance

Comparing bjmc:comparison-protocols (2673881) with main (28b36d9)

Summary

✅ 13 untouched benchmarks

@bjmc
Copy link
Contributor Author

bjmc commented May 22, 2024

The only test failures look to be with an older version of mypy. Maybe it doesn't fully recognize the protocols from annotated_types?

Copy link
Member

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

@bjmc,

Thanks, this looks great. Let's consult with @dmontagu re the test failures.

Comment on lines 63 to 66
multiple_of: annotated_types.MultipleOf | None
strict: bool | None
min_length: int | None
max_length: int | None
min_length: annotated_types.MinLen | None
max_length: annotated_types.MaxLen | None
Copy link
Member

Choose a reason for hiding this comment

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

For these, let's actually keep the original type (float and int) and add an additional union member as the annotated_types type. Ex:
multiple_of: int | annotated_types.MultipleOf | None

Copy link
Member

Choose a reason for hiding this comment

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

@bjmc,

Thinking about this more, let's also add some tests accordingly, verifying that MultipleOf works in this case (it might not, due to expectations on the pydantic-core end of things.

@dmontagu
Copy link
Contributor

@bjmc the SupportsX look like protocols that should work (though as @sydney-runkle mentioned, we should confirm that they actually do work).

But the MinLen, MaxLen, and MultipleOf seem to be dataclasses and not protocols, right? (Am I missing something?) Did you check if instances of these classes actually work? (I'd expect that they won't without further changes, but it's been a while since I looked at these bits of code.)

@bjmc
Copy link
Contributor Author

bjmc commented May 28, 2024

@dmontagu Good shout, I'll revert those.

@bjmc
Copy link
Contributor Author

bjmc commented May 28, 2024

I was overgeneralizing from the metadata_lookup here but you're absolutely right those are dataclasses to store the constraints, not Protocols to describe input types. I've reverted them now.

I suppose to be extremely pedantic, min/max lengths should be typed Annotated[int, Ge(0)] rather than just int but maybe we don't care about that.

@bjmc
Copy link
Contributor Author

bjmc commented May 28, 2024

@sydney-runkle - Confirming this fixes my issue, using the same example.py from #9472 :

bjmc@host:~/Sandbox/pydantic$ git checkout main 
Switched to branch 'main'
Your branch is up to date with 'origin/main'.
bjmc@host:~/Sandbox/pydantic$ pyright example.py 
/home/bjmc/Sandbox/pydantic/example.py
  /home/bjmc/Sandbox/pydantic/example.py:6:44 - error: Argument of type "timedelta" cannot be assigned to parameter "gt" of type "float | None" in function "Field"
    Type "timedelta" is incompatible with type "float | None"
      "timedelta" is incompatible with "float"
      "timedelta" is incompatible with "None" (reportArgumentType)
1 error, 0 warnings, 0 informations 
bjmc@host:~/Sandbox/pydantic$ git checkout comparison-protocols 
Switched to branch 'comparison-protocols'
Your branch is up to date with 'origin/comparison-protocols'.
bjmc@host:~/Sandbox/pydantic$ pyright example.py 
0 errors, 0 warnings, 0 informations 

@bjmc bjmc marked this pull request as ready for review May 28, 2024 22:13
@sydney-runkle
Copy link
Member

@bjmc,

Nice, the new changes look good. I think we do still want to add some explicit tests for the new protocols, but I'm not sure what the best way to do that is. I'll consult with @dmontagu and get back to you soon!

@sydney-runkle
Copy link
Member

@bjmc,

I spoke with @adriangb, our resident annotated types guru, and we've decided that this PR is good to go.

Awesome work here, thanks for your contribution!!

@sydney-runkle sydney-runkle merged commit fc87631 into pydantic:main May 29, 2024
52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes-fix Used for bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type signature for Field() gt/lt/gte/lte validators should include timedelta
3 participants