Include additional files to be typed checks, up to exceptions.py #1931

savarin · 2020-08-23T19:17:08Z

Addresses #1897, as per discussion in #1897 (comment).

This PR includes additional files to be typed checked, up to exceptions.py. Note that both the .py and .pyi files are included in the checks.

In particular, the .pyi file from typeshed for exceptions.py are ported over and expanded to satisfy type checks.

savarin · 2020-08-23T19:18:09Z

src/urllib3/exceptions.pyi

+
+class PoolError(HTTPError):
+    pool: Any
+    def __init__(self, pool: Any, message: str) -> None: ...


The pool here is from connectionpool.py, but importing here would create a circular dependency.

If you're hitting circular imports you can sometimes use this construction:

from typing import TYPE_CHECKING if TYPE_CHECKING: from urllib3.connectionpool import ConnectionPool class PoolError: pool: "ConnectionPool"

savarin · 2020-08-23T19:19:37Z

src/urllib3/exceptions.pyi

+class MaxRetryError(RequestError):
+    reason: Any
+    def __init__(self, pool: Any, url: str, reason=Optional[str]) -> None: ...  # type: ignore
+
+class HostChangedError(RequestError):
+    retries: Any
+    def __init__(self, pool: Any, url: str, retries=int) -> None: ...  # type: ignore


I'm a little puzzled why I keep getting a mypy error here despite adding explicit types, so included an ignore.

src/urllib3/exceptions.pyi:25: error: Function is missing a type annotation for one or more arguments src/urllib3/exceptions.pyi:29: error: Function is missing a type annotation for one or more arguments.

Should be retries: int instead of =int :)

LOL I guess this is where I should have taken a break :D

codecov · 2020-08-23T19:28:12Z

Codecov Report

Merging #1931 into master will increase coverage by 0.04%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1931      +/-   ##
==========================================
+ Coverage   99.77%   99.81%   +0.04%     
==========================================
  Files          24       24              
  Lines        2179     2179              
==========================================
+ Hits         2174     2175       +1     
+ Misses          5        4       -1

Flag	Coverage Δ
#unittests	`99.35% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/urllib3/util/ssl_.py	`100.00% <0.00%> (+1.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df1f648...10cb500. Read the comment docs.

sethmlarson

Thanks for this, here are some comments for you :)

I'm wondering about the empty .pyi files, should we follow the dependency tree to commit type stubs for all the dependencies of a node before that node itself to not run into issues?

noxfile.py

sethmlarson · 2020-08-23T21:23:41Z

src/urllib3/exceptions.pyi

+
+class PoolError(HTTPError):
+    pool: Any
+    def __init__(self, pool: Any, message: str) -> None: ...


If you're hitting circular imports you can sometimes use this construction:

from typing import TYPE_CHECKING if TYPE_CHECKING: from urllib3.connectionpool import ConnectionPool class PoolError: pool: "ConnectionPool"

sethmlarson · 2020-08-23T21:24:30Z

src/urllib3/exceptions.pyi

+
+class MaxRetryError(RequestError):
+    reason: Any
+    def __init__(self, pool: Any, url: str, reason=Optional[str]) -> None: ...  # type: ignore


Should be :Optional[...] instead of =Optional[...], remove the # type: ignore after verifying this works

sethmlarson · 2020-08-23T21:25:03Z

src/urllib3/exceptions.pyi

+class LocationValueError(ValueError, HTTPError): ...
+
+class LocationParseError(LocationValueError):
+    location: Any


I think we know location is a str from the constructor? Can change from Any to str

sethmlarson · 2020-08-23T21:26:13Z

src/urllib3/exceptions.py

@@ -1,5 +1,5 @@
 from __future__ import absolute_import
-from .packages.six.moves.http_client import IncompleteRead as httplib_IncompleteRead


I'm guessing this # type: ignore is temporary? Should we start by typing urllib3.packages.six per the dependency tree?

savarin · 2020-08-23T22:17:16Z

Thanks for this, here are some comments for you :)

I'm wondering about the empty .pyi files, should we follow the dependency tree to commit type stubs for all the dependencies of a node before that node itself to not run into issues?

Thank you for the comments! I think that resolves all the generic Any and type ignores.

I admit I took the easy way out with six.pyi, but what the type stubs would look like there complete baffles me. I took another quick look, if understand correctly we just need to stub out MovedModule and MovedAttribute?

savarin · 2020-08-23T22:52:01Z

@sethmlarson OK I took a stab at packages.six.pyi (as I understand it six resolves which module to use), it doesn't appear we need one for packages.__init__.pyi.

savarin · 2020-08-26T06:54:09Z

@sethmlarson My initial plan was to ping to see if you had any additional thoughts, but instead I added additional downstream files to TYPED_FILES to test out packages.six.pyi. Since there errors were raised, this shows that the stubs in packages.six.pyi is not having the desired effect.

I'll take another look tomorrow. The ones that I'm still a bit puzzled on how to stub are the ones like urllib3.packages.six.moves and urllib3.packages.six.moves.urllib.parse.

"src/urllib3/util/queue.py:3: error: Cannot find implementation or library stub for module named 'urllib3.packages.six.moves'"
"src/urllib3/contrib/_securetransport/bindings.py:48: error: Module 'urllib3.packages.six' has no attribute 'raise_from'"
"src/urllib3/filepost.py:9: error: Module 'urllib3.packages.six' has no attribute 'b'"
"src/urllib3/request.py:4: error: Cannot find implementation or library stub for module named 'urllib3.packages.six.moves.urllib.parse'"
"src/urllib3/_collections.py:21: error: Module 'urllib3.packages.six' has no attribute 'iterkeys'"
"src/urllib3/_collections.py:21: error: Module 'urllib3.packages.six' has no attribute 'itervalues'"
"src/urllib3/_collections.py:21: error: Module 'urllib3.packages.six' has no attribute 'PY3'"
"src/urllib3/util/response.py:2: error: Cannot find implementation or library stub for module named 'urllib3.packages.six.moves'"
"src/urllib3/util/request.py:4: error: Module 'urllib3.packages.six' has no attribute 'b'"
"src/urllib3/util/request.py:4: error: Module 'urllib3.packages.six' has no attribute 'integer_types'"
"src/urllib3/connection.py:10: error: Cannot find implementation or library stub for module named 'urllib3.packages.six.moves.http_client'"
"src/urllib3/response.py:26: error: Module 'urllib3.packages.six' has no attribute 'string_types'"
"src/urllib3/response.py:26: error: Module 'urllib3.packages.six' has no attribute 'PY3'"
"src/urllib3/connectionpool.py:28: error: Cannot find implementation or library stub for module named 'urllib3.packages.six.moves'"
"src/urllib3/poolmanager.py:20: error: Cannot find implementation or library stub for module named 'urllib3.packages.six.moves.urllib.parse'"
"src/urllib3/contrib/appengine.py:45: error: Cannot find implementation or library stub for module named 'urllib3.packages.six.moves.urllib.parse'"
"src/urllib3/contrib/ntlmpool.py:12: error: Cannot find implementation or library stub for module named 'urllib3.packages.six.moves.http_client'"

savarin · 2020-09-09T14:48:35Z

Merged as #1937 instead.

include files up to exceptions

9f33d16

savarin commented Aug 23, 2020

View reviewed changes

sethmlarson requested changes Aug 23, 2020

View reviewed changes

Seth comments

7165cdf

savarin requested a review from sethmlarson August 23, 2020 22:50

Merge remote-tracking branch 'upstream/master' into port-types-02

10cb500

savarin mentioned this pull request Aug 27, 2020

Include exceptions.pyi and fields.pyi #1937

Merged

savarin closed this Sep 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include additional files to be typed checks, up to exceptions.py #1931

Include additional files to be typed checks, up to exceptions.py #1931

savarin commented Aug 23, 2020

savarin Aug 23, 2020

sethmlarson Aug 23, 2020

savarin Aug 23, 2020 •

edited

sethmlarson Aug 23, 2020

savarin Aug 23, 2020

codecov bot commented Aug 23, 2020 •

edited

sethmlarson left a comment

sethmlarson Aug 23, 2020

sethmlarson Aug 23, 2020

sethmlarson Aug 23, 2020

sethmlarson Aug 23, 2020

savarin commented Aug 23, 2020 •

edited

savarin commented Aug 23, 2020 •

edited

savarin commented Aug 26, 2020 •

edited

savarin commented Sep 9, 2020

		@@ -1,5 +1,5 @@
		from __future__ import absolute_import
		from .packages.six.moves.http_client import IncompleteRead as httplib_IncompleteRead

Include additional files to be typed checks, up to exceptions.py #1931

Include additional files to be typed checks, up to exceptions.py #1931

Conversation

savarin commented Aug 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savarin Aug 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 23, 2020 • edited

Codecov Report

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

Choose a reason for hiding this comment

savarin commented Aug 23, 2020 • edited

savarin commented Aug 23, 2020 • edited

savarin commented Aug 26, 2020 • edited

savarin commented Sep 9, 2020

savarin Aug 23, 2020 •

edited

codecov bot commented Aug 23, 2020 •

edited

savarin commented Aug 23, 2020 •

edited

savarin commented Aug 23, 2020 •

edited

savarin commented Aug 26, 2020 •

edited