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

http: improve types #9055

Merged
merged 5 commits into from
Nov 3, 2022
Merged

http: improve types #9055

merged 5 commits into from
Nov 3, 2022

Conversation

JelleZijlstra
Copy link
Member

No description provided.

@github-actions

This comment has been minimized.

@@ -30,7 +30,7 @@ __all__ = [
"HTTPSConnection",
]

_DataType: TypeAlias = bytes | IO[Any] | Iterable[bytes] | str
_DataType: TypeAlias = SupportsRead[bytes] | Iterable[ReadableBuffer] | ReadableBuffer
Copy link
Collaborator

Choose a reason for hiding this comment

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

HTTPConnection.send() also allows str:

    def request(self, method, url, body=None, headers={}, *,
                encode_chunked=False):
        """Send a complete request to the server."""
        self._send_request(method, url, body, headers, encode_chunked)

    def _send_request(self, method, url, body, headers, encode_chunked):
        # ...
        # It also gets passed to _get_content_length(), which also supports str
        # ...

        if isinstance(body, str):
            # RFC 2616 Section 3.7.1 says that text default has a
            # default charset of iso-8859-1.
            body = _encode(body, 'body')
        self.endheaders(body, encode_chunked=encode_chunked)

I think the best solution is just to add str to the body argument of request manually.

Comment on lines 57 to 59
server: socketserver.BaseServer,
directory: str | None = ...,
index_pages: Sequence[str] | None = ...,
Copy link
Collaborator

Choose a reason for hiding this comment

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

As directory is keyword only and I can't find index_pages anywhere (on 3.9):

Suggested change
server: socketserver.BaseServer,
directory: str | None = ...,
index_pages: Sequence[str] | None = ...,
server: socketserver.BaseServer,
*,
directory: str | None = ...,

(directory is also passed to os.fspath() in later version (but not 3.7), but that's out of scope for this PR, since it needs overloads.)

Copy link
Member

@AlexWaygood AlexWaygood Nov 1, 2022

Choose a reason for hiding this comment

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

index_pages is new in 3.12: python/cpython#31985. So we should branch on if sys.version_info >= (3, 12).

@JelleZijlstra JelleZijlstra requested a review from srittau November 3, 2022 03:39
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@srittau srittau merged commit 6b70245 into python:main Nov 3, 2022
AlexWaygood added a commit that referenced this pull request Nov 3, 2022
The stub currently implies that the index_pages parameter has been removed in 3.12. Actually, it was only added in 3.12 :)

See python/cpython#31985. (This is a follow-up to #9055 — cc. @JelleZijlstra.)
@JelleZijlstra JelleZijlstra deleted the http branch November 3, 2022 14:21
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.

3 participants