-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add a with_config decorator to comply with typing spec
#8611
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
Conversation
CodSpeed Performance ReportMerging #8611 will not alter performanceComparing Summary
|
sydney-runkle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Viicos,
@dmontagu and I really like this approach. A few notes:
It'd be great if this could also be used with dataclasses. I think as it stands, it can, but you just haven't documented it yet. A few requests for changes (I know you wanted to get this approved before working on additional docs):
- Docs update with a vanilla example for both
dataclassandTypedDict. Docs in both the "Concepts" section and "API Documentation" section (via docstrings) would be fantastic. mypytests with this new decorator, as a proof of concept for how this fixes the original issue.
Thanks a bunch for your work on this - it'll be a great feature to have :).
|
Please review |
edaa36b to
d28b239
Compare
d28b239 to
645f7f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look great. I've requested two small docs updates. Additionally, looks like the mypy test isn't working properly. Could you please update that when you have a chance?
Other than that, looks good to go! Thanks!
|
I've moved the attached issue into the 2.7.0 milestone :). |
867bbbb to
1087ab1
Compare
1087ab1 to
7daacab
Compare
|
Amazing work 🚀. Merging this now to be included in 2.7.0! |
May fix #8514, as suggested in a comment.
If accepted, better documentation will follow
Selected Reviewer: @dmontagu