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

Fix the un-hashability of various annotation types, for use in caching generic containers #6480

Merged
merged 1 commit into from Jul 6, 2023

Conversation

dmontagu
Copy link
Contributor

@dmontagu dmontagu commented Jul 6, 2023

skip change file check

Selected Reviewer: @hramezani

@cloudflare-pages
Copy link

cloudflare-pages bot commented Jul 6, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: fee40b4
Status: ✅  Deploy successful!
Preview URL: https://02e16799.pydantic-docs2.pages.dev
Branch Preview URL: https://fix-unhashable-annotations.pydantic-docs2.pages.dev

View logs

@dmontagu
Copy link
Contributor Author

dmontagu commented Jul 6, 2023

Hmm thought about this a bit more and I think it might be better to not add TransformSchema to __all__ quite yet. I want to play with other possible tweaks to these annotations before merging this.

Copy link
Contributor

@ybressler ybressler left a comment

Choose a reason for hiding this comment

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

Note: I'm trying to get in the habit of reviewing more code here. Apologies if my comments are n00b.

Code looks straightforward overall. I appreciate the addition of tests. Your PR description mentions inclusion of methods in caching, though I don't see any tests which relate to the caching of these methods. (Unless they are implicit?)

Comment on lines 5116 to 5122
ValidateStrAsInt = Annotated[str, TransformSchema(lambda _s: core_schema.int_schema())]

class Model(BaseModel):
x: Optional[ValidateStrAsInt]

assert Model(x=None).x is None
assert Model(x='1').x == 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Transformation from a string repr of an int/float/decimal is no longer automatically executed in V2, right? I'm assuming you are overriding this with TransformSchema(lambda _s: core_schema.int_schema())?

Also, this test could be rewritten as parameterized if you wanted to go overboard. Perhaps some other edge cases like negative numbers, or floats? (Though would a float get transformed to an int?)

@dmontagu dmontagu force-pushed the fix-unhashable-annotations branch from 5b8c801 to 206dceb Compare July 6, 2023 15:19
@dmontagu dmontagu force-pushed the fix-unhashable-annotations branch from 206dceb to fee40b4 Compare July 6, 2023 15:23
@dmontagu
Copy link
Contributor Author

dmontagu commented Jul 6, 2023

Your PR description mentions inclusion of methods in caching, though I don't see any tests which relate to the caching of these methods. (Unless they are implicit?)

Optional[X] attempts to cache the result of Optional.__getitem__(X), and fails if X is not hashable. So the addition of Optional[SkipValidation[int]] etc. is how I am testing this. Note these tests do fail without the changes in this PR.

@dmontagu
Copy link
Contributor Author

dmontagu commented Jul 6, 2023

please review

@dmontagu dmontagu merged commit f8ba088 into main Jul 6, 2023
51 checks passed
@dmontagu dmontagu deleted the fix-unhashable-annotations branch July 6, 2023 18:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants