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

More accurate message types for email stubs #12104

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

max-muoto
Copy link
Contributor

@max-muoto max-muoto commented Jun 6, 2024

Fix long-standing email.parser issues detailed in #2417 and #10762

This takes the approach recommended by @andersk and utilizies generic defaults to help us get accurate types accounting for the default arguments here without any overloads.

@max-muoto max-muoto changed the title chore: Add defaults for email message funcs More accurate message types for email stubs Jun 6, 2024

This comment has been minimized.

3 similar comments

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@max-muoto max-muoto force-pushed the defaults-for-email-message-funcs branch from 441fb8d to 16ef0ee Compare June 6, 2024 20:29
@max-muoto max-muoto marked this pull request as ready for review June 6, 2024 20:32

This comment has been minimized.

@andersk
Copy link
Contributor

andersk commented Jun 7, 2024

Policy.message_factory, Policy.__init__(message_factory), Policy.handle_defect, Policy.register_defect should use _M rather than Message.

@max-muoto
Copy link
Contributor Author

Policy.message_factory, Policy.__init__(message_factory), Policy.handle_defect, Policy.register_defect should use _M rather than Message.

Sounds good, wasn't 100% sure there. Let me fix that.

@max-muoto
Copy link
Contributor Author

Policy.message_factory, Policy.__init__(message_factory), Policy.handle_defect, Policy.register_defect should use _M rather than Message.

c8e2982

This comment has been minimized.

@@ -1,17 +1,21 @@
# pyright: reportInvalidTypeVarUse=false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See microsoft/pyright#8084 for some context on why we need this.

@andersk
Copy link
Contributor

andersk commented Jun 7, 2024

Oh and EmailPolicy.__init__(message_factory) should return EmailMessage rather than Message.

@max-muoto
Copy link
Contributor Author

Oh and EmailPolicy.__init__(message_factory) should return EmailMessage rather than Message.

85ed295

This comment has been minimized.

@max-muoto
Copy link
Contributor Author

@srittau It seems like you looked into making Policy generic at some point? Do you maybe want to review here?

@max-muoto
Copy link
Contributor Author

Sorry to bug, but just wanted to follow-up again @srittau, or if maybe someone else is able to take a look.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

LGTM, a few optional suggestions below.

stdlib/email/__init__.pyi Outdated Show resolved Hide resolved
stdlib/email/_policybase.pyi Outdated Show resolved Hide resolved
stdlib/email/_policybase.pyi Outdated Show resolved Hide resolved
max-muoto and others added 3 commits July 15, 2024 11:34
Co-authored-by: Sebastian Rittau <srittau@rittau.biz>
Co-authored-by: Sebastian Rittau <srittau@rittau.biz>
Co-authored-by: Sebastian Rittau <srittau@rittau.biz>
@max-muoto
Copy link
Contributor Author

LGTM, a few optional suggestions below.

Thanks, accepted all of your suggestions.

Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

setuptools (https://github.com/pypa/setuptools)
+ setuptools/command/bdist_wheel.py:591: error: Argument "policy" to "Generator" has incompatible type "EmailPolicy"; expected "Optional[Policy[Message[str, str]]]"  [arg-type]

@srittau
Copy link
Collaborator

srittau commented Jul 15, 2024

Looking at the new primer hit, I think we need to make EmailMessage generic, too: class EmailMessage(MIMEPart[_HeaderRegistryT, _HeaderRegistryParamT]): ....

Although I have to admit, I'm a bit lost in how all these classes work together.

@max-muoto
Copy link
Contributor Author

Looking at the new primer hit, I think we need to make EmailMessage generic, too: class EmailMessage(MIMEPart[_HeaderRegistryT, _HeaderRegistryParamT]): ....

Although I have to admit, I'm a bit lost in how all these classes work together.

I'll take another deeper look in the next day or two, since it's a been a while since I worked on this originally now haha.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants