Skip to content

Fix email header types #1472

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 9, 2017
Merged

Fix email header types #1472

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions stdlib/3/email/message.pyi
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ from typing import (
import sys
from email.charset import Charset
from email.errors import MessageDefect
from email.header import Header
if sys.version_info >= (3, 3):
from email.policy import Policy
if sys.version_info >= (3, 4):
Expand All @@ -19,6 +20,7 @@ _PayloadType = Union[List[Message], str, bytes]
_CharsetType = Union[Charset, str, None]
_ParamsType = Union[str, None, Tuple[str, Optional[str], str]]
_ParamType = Union[str, Tuple[Optional[str], Optional[str], str]]
_HeaderType = Union[str, Header]

class Message:
preamble = ... # type: Optional[str]
Expand All @@ -36,16 +38,16 @@ class Message:
def get_charset(self) -> _CharsetType: ...
def __len__(self) -> int: ...
def __contains__(self, name: str) -> bool: ...
def __getitem__(self, name: str) -> Optional[str]: ...
def __setitem__(self, name: str, val: str) -> None: ...
def __getitem__(self, name: str) -> Optional[_HeaderType]: ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a Union return type a good choice here? See python/mypy#1693 for the issues with returning Union.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've had a look at that.

My concern is that a header is very different to a string: .lower(), .strip() etc all completely fail on a Header. There is little you can correctly do with the result of get/get_all/getitem without either checking the type, running str() on it, or passing it to another function that handles either type. Otherwise you will eventually hit type errors. This hit us at the Patchwork project when a header unexpectedly contained an invalid byte and our parser broke completely.

I'm happy to look at another way to approach the problem though - this is the first time I've touched typeshed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@matthiaskramm - any thoughts on how else I might approach this problem?

Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, I think a Union return type is probably the right call here based on @daxtens's arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following up on this - any further thoughts or suggestions on how I could do this differently?

Copy link
Member

Choose a reason for hiding this comment

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

@matthiaskramm what do you think? I'm OK with merging this as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, let's move forward with this.

def __setitem__(self, name: str, val: _HeaderType) -> None: ...
def __delitem__(self, name: str) -> None: ...
def keys(self) -> List[str]: ...
def values(self) -> List[str]: ...
def items(self) -> List[Tuple[str, str]]: ...
def get(self, name: str, failobj: _T = ...) -> Union[str, _T]: ...
def get_all(self, name: str, failobj: _T = ...) -> Union[List[str], _T]: ...
def values(self) -> List[_HeaderType]: ...
def items(self) -> List[Tuple[str, _HeaderType]]: ...
def get(self, name: str, failobj: _T = ...) -> Union[_HeaderType, _T]: ...
def get_all(self, name: str, failobj: _T = ...) -> Union[List[_HeaderType], _T]: ...
def add_header(self, _name: str, _value: str, **_params: _ParamsType) -> None: ...
def replace_header(self, _name: str, _value: str) -> None: ...
def replace_header(self, _name: str, _value: _HeaderType) -> None: ...
def get_content_type(self) -> str: ...
def get_content_maintype(self) -> str: ...
def get_content_subtype(self) -> str: ...
Expand Down