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 validate_call binding issue with inheritance #8258

Closed
wants to merge 3 commits into from

Conversation

sydney-runkle
Copy link
Member

@sydney-runkle sydney-runkle commented Nov 29, 2023

Change Summary

Fix #8146

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: @lig

@sydney-runkle sydney-runkle added the relnotes-fix Used for bugfixes. label Nov 29, 2023
Copy link

cloudflare-pages bot commented Nov 29, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6c7fd7f
Status: ✅  Deploy successful!
Preview URL: https://6bcb3843.pydantic-docs2.pages.dev
Branch Preview URL: https://fix-validate-call-issue.pydantic-docs2.pages.dev

View logs

Copy link

codspeed-hq bot commented Nov 29, 2023

CodSpeed Performance Report

Merging #8258 will not alter performance

Comparing fix-validate-call-issue (6c7fd7f) with main (667cd37)

Summary

✅ 10 untouched benchmarks

@sydney-runkle
Copy link
Member Author

@alexmojaki, what are your thoughts on this?

We've been debating a bit - on one hand the qualname approach is a bit dirty. On the other hand, we haven't yet figured out a clean approach (we tried a weakref cache, but couldn't get that working).

We want to have a solution that maintains the performance benefits of rebinding the wrapped function to an instance in general, while also catching this specific use case where we don't want to bind.

@sydney-runkle
Copy link
Member Author

Please review

@alexmojaki
Copy link
Contributor

ValidateCallWrapper says that it's a descriptor so that it "can be applied to instance methods, class methods, static methods, as well as normal functions." I don't understand this reasoning. In #8268 it looks like returning a plain function from @validate_call works fine*. The behaviour changes slightly, so maybe we could have a discussion about compatibility, but overall it feels like an improvement.

*(someone please rerun that failed pypy job though 😅)

@sydney-runkle
Copy link
Member Author

Closing in favor of #8268

@sydney-runkle sydney-runkle deleted the fix-validate-call-issue branch January 23, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

validate_call on method + inheritance bug
3 participants