Update mypy library version to 1.8.0 #3314

Nightrider0098 · 2024-01-25T02:42:25Z

No description provided.

pquentin

Thanks for tackling this!

pquentin · 2024-01-25T05:58:27Z

changelog/3312.feature.rst

@@ -0,0 +1 @@
+Updated mypy library version to 1.8.0


Since this is an internal change, we don't need a changelog entry. I've added the Skip Changelog to fix the CI check.

pquentin · 2024-01-25T06:03:15Z

src/urllib3/connection.py

@@ -307,7 +307,7 @@ def putrequest(
            method, url, skip_host=skip_host, skip_accept_encoding=skip_accept_encoding
        )

-    def putheader(self, header: str, *values: str) -> None:
+    def putheader(self, header: str | bytes, *values: str | bytes) -> None:


Our handling of bytes in headers is not ideal (#3072), but I would not want to block upgrading mypy because of it.

What error do you get without this change?

I'd say it's better to add # type: ignore instead of adding a type hint for an unsupported type

pquentin · 2024-01-25T06:06:27Z

src/urllib3/http2.py

@@ -72,11 +72,19 @@ def putrequest(
                )
            )

-    def putheader(self, header: str, *values: str) -> None:
+    def putheader(self, header: str | bytes, *values: str | bytes) -> None:


Same question, why did you need this?

pquentin · 2024-01-25T11:22:13Z

src/urllib3/contrib/pyopenssl.py

@@ -412,7 +412,12 @@ class PyOpenSSLContext:
    """

    def __init__(self, protocol: int) -> None:
-        self.protocol = _openssl_versions[protocol]
+        ssl_method = _openssl_versions.get(OpenSSL.SSL._SSLMethod(protocol))


The addition of OpenSSL.SSL._SSLMethod is breaking CI.

src/urllib3/contrib/pyopenssl.py

Co-authored-by: Illia Volochii <illia.volochii@gmail.com>

illia-v · 2024-01-26T13:32:11Z

src/urllib3/http2.py

-            self._h2_headers.append(
-                (header.encode("utf-8").lower(), value.encode("utf-8"))
-            )
+            if isinstance(header, str):
+                header_bytes = header.encode("utf-8").lower()
+            else:
+                header_bytes = header.lower()
+
+            if isinstance(value, str):
+                value_bytes = value.encode("utf-8")
+            else:
+                value_bytes = value
+
+            self._h2_headers.append((header_bytes, value_bytes))


Let's move adding support for non-str values out this PR

shall we throw error for other types? or add ignore @illia-v

In this PR no functional changes (new errors, etc.) are wanted because we don't add support for any new types

Cool will remove the other handling

illia-v · 2024-01-26T13:33:31Z

src/urllib3/contrib/pyopenssl.py

-        self.protocol = _openssl_versions[protocol]
+        ssl_method = _openssl_versions[protocol]
+        if ssl_method is None:
+            # Handle the case when protocol is not found
+            raise ValueError(f"Unsupported SSL/TLS protocol: {protocol}")
+        self.protocol = ssl_method


Please drop the change if it isn't required for successful type checking

illia-v · 2024-01-26T13:33:39Z

src/urllib3/contrib/pyopenssl.py



 __all__ = ["inject_into_urllib3", "extract_from_urllib3"]

+


Suggested change

src/urllib3/contrib/pyopenssl.py

Co-authored-by: Illia Volochii <illia.volochii@gmail.com>

Nightrider0098 · 2024-01-26T17:12:44Z

@pquentin Facing issue where i am getting test failure in random test. Like for the last commit it happend for 3.9 this time for macos. What could be the reason?

pquentin · 2024-01-26T19:15:31Z

We have some flakiness issues. If 90% of tests are passing and lint is passing you are probably OK. We will double check when reviewing.

Nightrider0098 · 2024-01-27T12:37:01Z

We have some flakiness issues. If 90% of tests are passing and lint is passing you are probably OK. We will double check when reviewing.

Have made all the required changes. Should we merge it?

sethmlarson

Thanks for this @Nightrider0098, here's my review:

sethmlarson · 2024-01-27T16:17:42Z

src/urllib3/contrib/pyopenssl.py

@@ -412,7 +412,8 @@ class PyOpenSSLContext:
    """

    def __init__(self, protocol: int) -> None:
-        self.protocol = _openssl_versions[protocol]
+        ssl_method = _openssl_versions[protocol]


Is this change needed now since _openssl_versions has been annotated?

sethmlarson · 2024-01-27T16:19:11Z

src/urllib3/http2.py

-            self._h2_headers.append(
-                (header.encode("utf-8").lower(), value.encode("utf-8"))
-            )
+            if isinstance(header, str):


I'm surprising this isn't giving a type error, if header or value aren't strings then header_bytes and value_bytes aren't defined? Should we revert the change adding the isinstance checks?

Remove is instance check and add type override

illia-v

LGTM, thanks! @Nightrider0098 feel free to claim the $100 bounty from our OpenCollective.

Update mypy library version to 1.8.0

8b83702

Nightrider0098 mentioned this pull request Jan 25, 2024

Upgrade mypy to the latest version in CI #3312

Closed

Add change log

80cf83a

pquentin added the Skip Changelog Pull requests that don't require a changelog entry label Jan 25, 2024

pquentin reviewed Jan 25, 2024

View reviewed changes

Nightrider0098 added 2 commits January 25, 2024 17:30

remove changelog

7203ba4

Add fix for type

edfafa9

illia-v reviewed Jan 26, 2024

View reviewed changes

src/urllib3/contrib/pyopenssl.py Outdated Show resolved Hide resolved

illia-v linked an issue Jan 26, 2024 that may be closed by this pull request

Upgrade mypy to the latest version in CI #3312

Closed

Nightrider0098 and others added 3 commits January 26, 2024 18:30

Update src/urllib3/contrib/pyopenssl.py

4886621

Co-authored-by: Illia Volochii <illia.volochii@gmail.com>

Add recommended changes

afd60ca

Fix recommended changs

c8e3b8e

illia-v reviewed Jan 26, 2024

View reviewed changes

Nightrider0098 and others added 5 commits January 26, 2024 19:12

Remove addiitonal checks for ssl_methods

81d68f3

Remove new line

a8d855b

Update src/urllib3/contrib/pyopenssl.py

d0bac0f

Co-authored-by: Illia Volochii <illia.volochii@gmail.com>

resolve comments

02fb2bc

Merge branch 'main' into updage-mypy-1.8.0

3b5a061

sethmlarson requested review from illia-v and pquentin January 27, 2024 16:16

sethmlarson requested changes Jan 27, 2024

View reviewed changes

Nightrider0098 and others added 3 commits January 28, 2024 01:22

Resolve comments

8bb3338

Resolve comments

d435104

Merge branch 'main' into updage-mypy-1.8.0

d7cdffa

illia-v approved these changes Jan 30, 2024

View reviewed changes

illia-v merged commit 60c4647 into urllib3:main Jan 30, 2024
28 of 31 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update mypy library version to 1.8.0 #3314

Update mypy library version to 1.8.0 #3314

Nightrider0098 commented Jan 25, 2024

pquentin left a comment

pquentin Jan 25, 2024

pquentin Jan 25, 2024

Nightrider0098 Jan 25, 2024

illia-v Jan 26, 2024

pquentin Jan 25, 2024

pquentin Jan 25, 2024

illia-v Jan 26, 2024

Nightrider0098 Jan 26, 2024

illia-v Jan 26, 2024

Nightrider0098 Jan 26, 2024 •

edited

Loading

illia-v Jan 26, 2024

illia-v Jan 26, 2024

Nightrider0098 commented Jan 26, 2024

pquentin commented Jan 26, 2024

Nightrider0098 commented Jan 27, 2024

sethmlarson left a comment

sethmlarson Jan 27, 2024

sethmlarson Jan 27, 2024

Nightrider0098 Jan 30, 2024

illia-v left a comment

Update mypy library version to 1.8.0 #3314

Update mypy library version to 1.8.0 #3314

Conversation

Nightrider0098 commented Jan 25, 2024

pquentin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nightrider0098 Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nightrider0098 commented Jan 26, 2024

pquentin commented Jan 26, 2024

Nightrider0098 commented Jan 27, 2024

sethmlarson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illia-v left a comment

Choose a reason for hiding this comment

Nightrider0098 Jan 26, 2024 •

edited

Loading