Fixed: Argument for Werkzeug's Headers.pop method. #5335
Conversation
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
| @@ -191,7 +191,7 @@ class Headers(object): | |||
| def extend(self, iterable): ... | |||
| def __delitem__(self, key: Any) -> None: ... | |||
| def remove(self, key): ... | |||
| def pop(self, **kwargs): ... | |||
| def pop(self, key: str, default: Optional[str] = ...): ... | |||
There was a problem hiding this comment.
From the implementation, looks like the key can also be an int or None.
Also, default can really be of any type. For a precise return type you'll probably need two overloads: one that doesn't take default as an argument and returns str, and one that does and returns the Union of the default's type and the normal return type. get() already had some similar overloads, though it's more complicated because of other parameters.
There was a problem hiding this comment.
@overload
def pop(self, key: str, default: Optional[str] = ...) -> str: ...
@overload
def pop(self, key: Optional[int], default: Optional[Tuple[str, str]] = ...) -> Tuple[str, str]: ...What if I do something like this ? This is similar to the types present in the original code. Also, I will need to handle this for the ImmutableHeadersMixin class's pop method as well.
There was a problem hiding this comment.
Interesting, didn't realize the return type changed if the key is an int. Yes, let's just go with the same types the library itself provides.
We'll eventually drop the Werkzeug stubs in typeshed once there's a release with its own types (#5337).
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
| @overload | ||
| def pop(self, key: str, default: Optional[str] = ...) -> str: ... | ||
| @overload | ||
| def pop(self, key: Optional[int], default: Optional[Tuple[str, str]] = ...) -> Tuple[str, str]: ... |
There was a problem hiding this comment.
This method seems to be something completely different than the other pop. It is defined as:
def pop(self, index=-1):
is_immutable(self)
While the other pop is like dict.pop, accepting key and default, this is more like list.pop.
There was a problem hiding this comment.
Ok got that. I only added that method as they handled that in their original code here. But, I guess what you said makes more sense.
|
I made all the changes as suggested in the comments. Learned a lot today. Still a noob in typing, but will keep learning and improving. |
|
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
Closes #5147