Update definition of constants in urllib3.util.retry #6892

bugant · 2022-01-11T12:41:06Z

This PR adds the latest naming for urllib3.util.retry default constants.

AlexWaygood · 2022-01-11T12:59:51Z

stubs/urllib3/urllib3/util/retry.pyi

@@ -12,8 +12,15 @@ ResponseError = exceptions.ResponseError
 log: Any

 class Retry:
+    DEFAULT_ALLOWED_METHODS: frozenset[str]
+    RETRY_AFTER_STATUS_CODES: frozenset[int]
+    DEFAULT_REMOVE_HEADERS_ON_REDIRECT: frozenset[list[str]]


Suggested change

DEFAULT_REMOVE_HEADERS_ON_REDIRECT: frozenset[list[str]]

DEFAULT_REMOVE_HEADERS_ON_REDIRECT: frozenset[str]

Looks like a frozenset with a single element to me, the string "Authorization" 🙂 (It's actually impossible to have a frozenset of lists, since lists aren't hashable!) https://github.com/urllib3/urllib3/blob/33afb5c96c435ab7118b5f41e26f96c290fef55b/src/urllib3/util/retry.py#L192

Yes good point @AlexWaygood! My bad. I will fix it now. Thank you

AlexWaygood · 2022-01-11T13:06:17Z

stubs/urllib3/urllib3/util/retry.pyi

@@ -12,8 +12,15 @@ ResponseError = exceptions.ResponseError
 log: Any

 class Retry:
+    DEFAULT_ALLOWED_METHODS: frozenset[str]


Suggested change

DEFAULT_ALLOWED_METHODS: frozenset[str]

DEFAULT_ALLOWED_METHODS: ClassVar[frozenset[str]]

Since these are class-level constants, it might be good to have them all as ClassVars (should apply similar changes to the other constants as well, if so)

AlexWaygood · 2022-01-11T13:16:39Z

Wait, do the deprecated constants in the stub even exist anymore? I can't see them in the source code anywhere...

bugant · 2022-01-11T13:21:22Z

do the deprecated constants in the stub even exist anymore? I can't see them in the source

They don't exists anymore... I'm not sure how to deal with those to be honest. I mean, probably some people using older version can still have those as valid default constants.

What do you suggest to do @AlexWaygood ?

AlexWaygood · 2022-01-11T13:23:03Z

do the deprecated constants in the stub even exist anymore? I can't see them in the source

They don't exists anymore... I'm not sure how to deal with those to be honest. I mean, probably some people using older version can still have those as valid default constants.

What do you suggest to do @AlexWaygood ?

Typeshed generally only ships stubs for the most recent version of a package, so I think they should probably be deleted. Looks like they were removed from urllib3 in November 2020! urllib3/urllib3@c67c094#diff-53a96dd57c993a39101318e9df47f8aab6610e8c2c1064f8109f66943dcbbd78

bugant · 2022-01-11T13:23:10Z

do the deprecated constants in the stub even exist anymore? I can't see them in the source

They don't exists anymore... I'm not sure how to deal with those to be honest. I mean, probably some people using older version can still have those as valid default constants.

What do you suggest to do @AlexWaygood ?

Those constants have been renamed here urllib3/urllib3@382ab32

bugant · 2022-01-11T13:34:36Z

do the deprecated constants in the stub even exist anymore? I can't see them in the source

They don't exists anymore... I'm not sure how to deal with those to be honest. I mean, probably some people using older version can still have those as valid default constants.
What do you suggest to do @AlexWaygood ?

Typeshed generally only ships stubs for the most recent version of a package, so I think they should probably be deleted. Looks like they were removed from urllib3 in November 2020! urllib3/urllib3@c67c094#diff-53a96dd57c993a39101318e9df47f8aab6610e8c2c1064f8109f66943dcbbd78

I removed deprecated constants and also used more precise types for the rest of the attributes (adding missing ones, and re-naming some of them).

AlexWaygood · 2022-01-11T13:39:47Z

I removed deprecated constants and also used more precise types for the rest of the attributes (adding missing ones, and re-naming some of them).

Looks great! Is there a reason why you've annotated some of the instance attributes differently to how they're annotated in the source code? E.g. allowed_methods is Optional[Collection[str]] in the source code, so could be Collection[str] | None here — is there a reason for it not to be?

bugant · 2022-01-11T13:42:37Z

I removed deprecated constants and also used more precise types for the rest of the attributes (adding missing ones, and re-naming some of them).

Looks great! Is there a reason why you've annotated some of the instance attributes differently to how they're annotated in the source code? E.g. allowed_methods is Optional[Collection[str]] in the source code, so could be Collection[str] | None here — is there a reason for it not to be?

My bad, I looked at an old version of the code, will fix it now. Thanks.

AlexWaygood · 2022-01-11T13:48:11Z

While we're cleaning up this file, could you also possibly remove the is_forced_retry method? It doesn't appear to exist at runtime; I'm not sure when it was removed, but I can't find any reference in the source code going back as far as 2018. Removing it would allow us to get rid of several entries from the stubtest allowlist for urllib3 (stubtest checks for inconsistencies between the stub and the runtime)

bugant · 2022-01-11T14:02:40Z

While we're cleaning up this file, could you also possibly remove the is_forced_retry method? It doesn't appear to exist at runtime; I'm not sure when it was removed, but I can't find any reference in the source code going back as far as 2018. Removing it would allow us to get rid of several entries from the stubtest allowlist for urllib3 (stubtest checks for inconsistencies between the stub and the runtime)

Yes @AlexWaygood , I did update all the methods as well. I'm not sure how to update the stubtest bit. Any hint?

bugant · 2022-01-11T14:03:57Z

While we're cleaning up this file, could you also possibly remove the is_forced_retry method? It doesn't appear to exist at runtime; I'm not sure when it was removed, but I can't find any reference in the source code going back as far as 2018. Removing it would allow us to get rid of several entries from the stubtest allowlist for urllib3 (stubtest checks for inconsistencies between the stub and the runtime)

Yes @AlexWaygood , I did update all the methods as well. I'm not sure how to update the stubtest bit. Any hint?

Ah the CI is reporting those, I think. I'm fixing that.

AlexWaygood · 2022-01-11T14:04:57Z

You'll need to remove the following entries from the stubtest allowlist as part of your PR (the test fails if there are unused entries):

urllib3.Retry.is_forced_retry
urllib3.connectionpool.Retry.is_forced_retry
urllib3.util.Retry.is_forced_retry
urllib3.util.retry.Retry.is_forced_retry

There are some other stubtest failures as well, which I'll help you out with.

AlexWaygood · 2022-01-11T14:06:16Z

I don't know why black is failing FWIW; the bot should have the power to push to your branch and autoformat your PR for you.

JelleZijlstra · 2022-01-11T14:08:33Z

the bot should have the power to push to your branch

That's not necessarily true; there's a checkbox somewhere to allow edits by maintainers and it looks like @bugant didn't turn it on.

AlexWaygood · 2022-01-11T14:08:33Z

stubs/urllib3/urllib3/util/retry.pyi

    ) -> None: ...
    def new(self, **kw): ...
    @classmethod
-    def from_int(cls, retries, redirect=..., default=...): ...
-    def get_backoff_time(self): ...
+    def from_int(cls, retries, redirect=..., default=...) -> "Retry": ...


No need to have string-literal annotations in stubs; from __future__ import annotations behaviour is enabled by default 🙂 same applies for the increment method

Suggested change

def from_int(cls, retries, redirect=..., default=...) -> "Retry": ...

def from_int(cls, retries, redirect=..., default=...) -> Retry: ...

BUT, on second thought, the annotation in the source code is wrong. You should annotate the method like this: add from _typeshed import Self at the top of the file, and then do:

Suggested change

def from_int(cls, retries, redirect=..., default=...) -> "Retry": ...

def from_int(cls: type[Self], retries, redirect=..., default=...) -> Self: ...

AlexWaygood · 2022-01-11T14:09:15Z

the bot should have the power to push to your branch

That's not necessarily true; there's a checkbox somewhere to allow edits by maintainers and it looks like @bugant didn't turn it on.

Thought that was probably it, but wasn't sure. Thanks!

bugant · 2022-01-11T14:22:50Z

Not sure which checkbox I have to tick

JelleZijlstra · 2022-01-11T14:26:58Z

It shows up when you create the PR ("Allow edits and access to secrets by maintainers"). Not sure if there's a way to change it later, so you may just have to run Black manually.

AlexWaygood · 2022-01-11T16:57:29Z

stubs/urllib3/urllib3/util/retry.pyi

+    backoff_factor: float
+    raise_on_redirect: bool
+    raise_on_status: bool
+    history: tuple[Any] | None


The attribute can never be None at runtime; if the None default in the __init__ method is used, then the attribute is set to an empty tuple.

You'll need to add back the RequestHistory NamedTuple you had in one of your previous commits (it's a collections.namedtuple in the 1.26.x branch, but it can be a typing.NamedTuple in the stub, like it is in the main branch; type-checkers don't care about the different kinds of NamedTuples)

Suggested change

history: tuple[Any] | None

history: tuple[()] | tuple[RequestHistory, ...]

AlexWaygood · 2022-01-11T17:02:56Z

stubs/urllib3/urllib3/util/retry.pyi

+    status: int | None
+    other: int | None
+    allowed_methods: Collection[str] | None
+    status_forcelist: Collection[int] | None


This also can never be None at runtime; it is set as an empty set if no value is provided in __init__.

Suggested change

status_forcelist: Collection[int] | None

status_forcelist: Collection[int]

AlexWaygood · 2022-01-11T17:19:02Z

stubs/urllib3/urllib3/util/retry.pyi

+    raise_on_status: bool
+    history: tuple[Any] | None
+    respect_retry_after_header: bool
+    remove_headers_on_redirect: Collection[str]


Any Collection can be passed into __init__, but it is changed to be a frozenset when it is set as an attribute on the class.

Suggested change

remove_headers_on_redirect: Collection[str]

remove_headers_on_redirect: frozenset[str]

AlexWaygood · 2022-01-11T17:21:30Z

stubs/urllib3/urllib3/util/retry.pyi

+    total: bool | int | None
+    connect: int | None
+    read: int | None
+    redirect: bool | int | None


It appears that redirect can never be False, only int, None or True. You'll need to import Literal from typing_extensions.

Suggested change

redirect: bool | int | None

redirect: Literal[True] | int | None

AlexWaygood · 2022-01-11T17:24:28Z

stubs/urllib3/urllib3/util/retry.pyi

@@ -44,7 +53,10 @@ class Retry:
    @classmethod
    def from_int(cls, retries, redirect=..., default=...): ...
    def get_backoff_time(self): ...
-    def sleep(self, response: HTTPResponse | None = ...) -> None: ...
-    def is_forced_retry(self, method, status_code): ...
+    def parse_retry_after(self, retry_after: str): ...


Suggested change

def parse_retry_after(self, retry_after: str): ...

def parse_retry_after(self, retry_after: str) -> float: ...

AlexWaygood · 2022-01-11T17:24:59Z

stubs/urllib3/urllib3/util/retry.pyi

-    def sleep(self, response: HTTPResponse | None = ...) -> None: ...
-    def is_forced_retry(self, method, status_code): ...
+    def parse_retry_after(self, retry_after: str): ...
+    def get_retry_after(self, response: HTTPResponse): ...


Suggested change

def get_retry_after(self, response: HTTPResponse): ...

def get_retry_after(self, response: HTTPResponse) -> float | None: ...

AlexWaygood · 2022-01-11T17:25:35Z

stubs/urllib3/urllib3/util/retry.pyi

-    def is_forced_retry(self, method, status_code): ...
+    def parse_retry_after(self, retry_after: str): ...
+    def get_retry_after(self, response: HTTPResponse): ...
+    def sleep_for_retry(self, response: HTTPResponse | None = ...): ...


Suggested change

def sleep_for_retry(self, response: HTTPResponse | None = ...): ...

def sleep_for_retry(self, response: HTTPResponse | None = ...) -> bool: ...

AlexWaygood · 2022-01-11T17:25:53Z

stubs/urllib3/urllib3/util/retry.pyi

+    def parse_retry_after(self, retry_after: str): ...
+    def get_retry_after(self, response: HTTPResponse): ...
+    def sleep_for_retry(self, response: HTTPResponse | None = ...): ...
+    def sleep(self, response: HTTPResponse | None = ...): ...


Suggested change

def sleep(self, response: HTTPResponse | None = ...): ...

def sleep(self, response: HTTPResponse | None = ...) -> None: ...

AlexWaygood · 2022-01-11T17:26:16Z

stubs/urllib3/urllib3/util/retry.pyi

+    def get_retry_after(self, response: HTTPResponse): ...
+    def sleep_for_retry(self, response: HTTPResponse | None = ...): ...
+    def sleep(self, response: HTTPResponse | None = ...): ...
+    def is_retry(self, method: str, status_code: int, has_retry_after: bool = ...): ...


Suggested change

def is_retry(self, method: str, status_code: int, has_retry_after: bool = ...): ...

def is_retry(self, method: str, status_code: int, has_retry_after: bool = ...) -> bool: ...

AlexWaygood

There's still more work to be done to finish annotating many of the methods in this file, but the changes you've made LGTM! (I don't have the power to merge, I'm just a contributor, not a maintainer.)

JelleZijlstra · 2022-01-11T20:57:15Z

stubs/urllib3/urllib3/util/retry.pyi

+    backoff_factor: float
+    raise_on_redirect: bool
+    raise_on_status: bool
+    history: tuple[RequestHistory, ...] | tuple[()]


Why do we need the empty tuple called out here? The first union member is an arbitrary-length tuple, which includes the empty tuple.

Oh that's my fault; I suggested that annotation, and didn't think that one through. My bad; you're quite right.

bugant · 2022-01-11T21:47:09Z

There's still more work to be done to finish annotating many of the methods in this file, but the changes you've made LGTM! (I don't have the power to merge, I'm just a contributor, not a maintainer.)

Thanks @AlexWaygood! What else is left there? I intentionally left out methods whose name starts with an _, thinking that those are kind of "private"/internal.

AlexWaygood · 2022-01-11T21:52:18Z

There's still more work to be done to finish annotating many of the methods in this file, but the changes you've made LGTM! (I don't have the power to merge, I'm just a contributor, not a maintainer.)

Thanks @AlexWaygood! What else is left there?

The remaining changes can be made in other PRs! But this is what's still left to do -- feel free to add it in this PR if you like!

None of the parameters in Retry.__init__, Retry.from_int, or Retry.increment have annotations yet.
Retry.new is lacking annotations for its parameters and its return type.
Retry.get_backoff_time is missing a return annotation.
We can probably do better than Any for the global log variable?

AlexWaygood · 2022-01-11T21:56:12Z

The log variable is actually pretty obviously a logging.Logger object: https://github.com/urllib3/urllib3/blob/b1f60e44d43b13e5272d5b6003f125af9c25c8ad/src/urllib3/util/retry.py#L22

bugant · 2022-01-12T08:17:08Z

There's still more work to be done to finish annotating many of the methods in this file, but the changes you've made LGTM! (I don't have the power to merge, I'm just a contributor, not a maintainer.)

Thanks @AlexWaygood! What else is left there?

The remaining changes can be made in other PRs! But this is what's still left to do -- feel free to add it in this PR if you like!
* None of the parameters in `Retry.__init__`, `Retry.from_int`, or `Retry.increment` have annotations yet.

* `Retry.new` is lacking annotations for its parameters and its return type.

* `Retry.get_backoff_time` is missing a return annotation.

* We can probably do better than `Any` for the global `log` variable?

Thanks @AlexWaygood , I added the missing annotatiions as you suggested.

AlexWaygood · 2022-01-12T08:23:23Z

stubs/urllib3/urllib3/util/retry.pyi

    ) -> None: ...
-    def new(self, **kw): ...
+    def new(self, **kw: Any) -> Retry: ...


At runtime this will return an instance of a subclass if it's being called on a subclass of Retry, so we should use a TypeVar to annotate the return-type instead of hardcoding it. (This is different to how they've annotated their code on main, but I think their annotation is wrong.) typeshed's established style is to use the _typeshed.Self TypeVar for these situations (you'll need to do from _typeshed import Self at the top of the file).

Suggested change

def new(self, **kw: Any) -> Retry: ...

def new(self: Self, **kw: Any) -> Self: ...

AlexWaygood · 2022-01-12T08:25:30Z

Thanks @AlexWaygood , I added the missing annotatiions as you suggested.

Thank you!!

AlexWaygood · 2022-01-12T08:28:36Z

stubs/urllib3/urllib3/util/retry.pyi

    ) -> None: ...
-    def new(self, **kw): ...
+    def new(self, **kw: Any) -> Self: ...


Suggested change

def new(self, **kw: Any) -> Self: ...

def new(self: Self, **kw: Any) -> Self: ...

srittau

Thanks!

bugant · 2022-01-12T09:42:59Z

@AlexWaygood do you know if it is possible to install types-urllib3 from the master branch? I cannot understand how to do it

srittau · 2022-01-12T09:48:22Z

@bugant: New versions of the stubs are released automatically every three hours, starting midnight UTC. So the next version of the package should be released in a bit over two hours. I unfortunately just missed the last release by about five minutes.

srittau · 2022-01-12T09:50:12Z

Actually, I just manually forced urllib3 to update, types-urllib3 1.26.6 should have these changes.

bugant · 2022-01-12T09:53:41Z

Actually, I just manually forced urllib3 to update, types-urllib3 1.26.6 should have these changes.

Fabulous! Thank you very much @srittau!

Update definition of constants in urllib3.util.retry

dc5908e

bugant force-pushed the update-urllib3-stubs branch from 72c4d6c to dc5908e Compare January 11, 2022 12:56

AlexWaygood reviewed Jan 11, 2022

View reviewed changes

DEFAULT_REMOVE_HEADERS_ON_REDIRECT is a frozenset[str]

3590543

AlexWaygood reviewed Jan 11, 2022

View reviewed changes

Mark default values as ClassVar

d37248a

Remove deprecated constants and add more precise types for attributes

95cb6ce

bugant added 2 commits January 11, 2022 14:56

Update attributes and methods typing

e8e45d9

Use nex syntax for typing

690c081

Removing is_forced_retry unused stubs from allowedlist

7c87b9d

AlexWaygood reviewed Jan 11, 2022

View reviewed changes

bugant added 3 commits January 11, 2022 15:14

Set default parameter values in methods

7cd4351

Fix black

9b885ff

Fixing flake8 issues

13a0914

AlexWaygood reviewed Jan 11, 2022

View reviewed changes

bugant added 3 commits January 11, 2022 18:41

Make type more precise and add return types in methods

4e509dd

Proper class definition for RequestHistory

7960ed1

Fix black

a604f0c

AlexWaygood approved these changes Jan 11, 2022

View reviewed changes

JelleZijlstra reviewed Jan 11, 2022

View reviewed changes

bugant added 2 commits January 12, 2022 09:09

Add missing type annotations in methods

c4ca733

format code with black

298f89f

AlexWaygood reviewed Jan 12, 2022

View reviewed changes

Type new to be correct for sub-classes

bf3f7bf

AlexWaygood reviewed Jan 12, 2022

View reviewed changes

Add missing type for self

39296b9

srittau approved these changes Jan 12, 2022

View reviewed changes

srittau merged commit 6907d7f into python:master Jan 12, 2022

JelleZijlstra mentioned this pull request Jan 12, 2022

Remove urllib3 types (not before October 2023) #6898

Closed

AlexWaygood mentioned this pull request Jan 13, 2022

requests: remove an unused allowlist entry #6911

Merged

AlexWaygood mentioned this pull request Jan 21, 2022

Detect unhashable elements in a set? PyCQA/flake8-pyi#124

Closed

	DEFAULT_REMOVE_HEADERS_ON_REDIRECT: frozenset[list[str]]
	DEFAULT_REMOVE_HEADERS_ON_REDIRECT: frozenset[str]

	DEFAULT_ALLOWED_METHODS: frozenset[str]
	DEFAULT_ALLOWED_METHODS: ClassVar[frozenset[str]]

	def from_int(cls, retries, redirect=..., default=...) -> "Retry": ...
	def from_int(cls, retries, redirect=..., default=...) -> Retry: ...

	def from_int(cls, retries, redirect=..., default=...) -> "Retry": ...
	def from_int(cls: type[Self], retries, redirect=..., default=...) -> Self: ...

	history: tuple[Any] \| None
	history: tuple[()] \| tuple[RequestHistory, ...]

	status_forcelist: Collection[int] \| None
	status_forcelist: Collection[int]

	remove_headers_on_redirect: Collection[str]
	remove_headers_on_redirect: frozenset[str]

	redirect: bool \| int \| None
	redirect: Literal[True] \| int \| None

	def parse_retry_after(self, retry_after: str): ...
	def parse_retry_after(self, retry_after: str) -> float: ...

	def get_retry_after(self, response: HTTPResponse): ...
	def get_retry_after(self, response: HTTPResponse) -> float \| None: ...

	def sleep_for_retry(self, response: HTTPResponse \| None = ...): ...
	def sleep_for_retry(self, response: HTTPResponse \| None = ...) -> bool: ...

	def sleep(self, response: HTTPResponse \| None = ...): ...
	def sleep(self, response: HTTPResponse \| None = ...) -> None: ...

	def is_retry(self, method: str, status_code: int, has_retry_after: bool = ...): ...
	def is_retry(self, method: str, status_code: int, has_retry_after: bool = ...) -> bool: ...

	def new(self, **kw: Any) -> Retry: ...
	def new(self: Self, **kw: Any) -> Self: ...

	def new(self, **kw: Any) -> Self: ...
	def new(self: Self, **kw: Any) -> Self: ...

Update definition of constants in urllib3.util.retry #6892

Update definition of constants in urllib3.util.retry #6892

Conversation

bugant commented Jan 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexWaygood commented Jan 11, 2022

bugant commented Jan 11, 2022

AlexWaygood commented Jan 11, 2022

bugant commented Jan 11, 2022

bugant commented Jan 11, 2022

AlexWaygood commented Jan 11, 2022 • edited Loading

bugant commented Jan 11, 2022

AlexWaygood commented Jan 11, 2022 • edited Loading

bugant commented Jan 11, 2022

bugant commented Jan 11, 2022

AlexWaygood commented Jan 11, 2022

AlexWaygood commented Jan 11, 2022

JelleZijlstra commented Jan 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexWaygood commented Jan 11, 2022

bugant commented Jan 11, 2022

JelleZijlstra commented Jan 11, 2022

AlexWaygood Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

AlexWaygood Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexWaygood Jan 11, 2022 • edited Loading

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

AlexWaygood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexWaygood Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

bugant commented Jan 11, 2022 • edited Loading

AlexWaygood commented Jan 11, 2022 • edited Loading

AlexWaygood commented Jan 11, 2022

bugant commented Jan 12, 2022

AlexWaygood Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

AlexWaygood commented Jan 12, 2022

Choose a reason for hiding this comment

srittau left a comment

Choose a reason for hiding this comment

bugant commented Jan 12, 2022

srittau commented Jan 12, 2022

srittau commented Jan 12, 2022

bugant commented Jan 12, 2022

AlexWaygood commented Jan 11, 2022 •

edited

Loading

AlexWaygood commented Jan 11, 2022 •

edited

Loading

AlexWaygood Jan 11, 2022 •

edited

Loading

AlexWaygood Jan 11, 2022 •

edited

Loading

AlexWaygood Jan 11, 2022 •

edited

Loading

AlexWaygood Jan 11, 2022 •

edited

Loading

bugant commented Jan 11, 2022 •

edited

Loading

AlexWaygood commented Jan 11, 2022 •

edited

Loading

AlexWaygood Jan 12, 2022 •

edited

Loading