Added support for union operators to HTTPHeaderDict
#2943
Conversation
|
I definitely like this feature, thank you for creating this PR. I'll give it a complete review soon, but for now could you run |
|
@sethmlarson I've addressed the coverage miss. edit: I asked around some colleagues about this. It seems like it's because, unlike dict, This would appease the type checker in tests, and has the advantage of being simple/non-intrusive. The drawback is that mypy would also complain for any user code using the same construct (users may also have to add the cast if they want to use type checking). The other option may be to update the annotation of |
|
Other options can be I think requiring the union operator support in the annotation in urllib3/src/urllib3/_request_methods.py Line 52 in 10ceef1 |
src/urllib3/_collections.py
Outdated
| maybe_constructable = ensure_can_construct_http_header_dict(other) | ||
| if maybe_constructable is None: | ||
| return NotImplemented | ||
| other_as_http_header_dict = type(self)(maybe_constructable) |
There was a problem hiding this comment.
Are there any advantages of the conversion in __ior__ and __or__?
There was a problem hiding this comment.
IIRC mypy was unhappy about that, because __ior__ and __or__ must accept any arbitrary object as "other" (and will opt-out of the comparison by returning NotImplemented), but HTTPHeaderDict.extend explicitly wants a ValidHTTPHeaderSource.
There was a problem hiding this comment.
This creates new objects and looks to have little functional benefit because extend will do similar kind of validation. Can typing.cast be used instead?
There was a problem hiding this comment.
Yes, likely. I'll try it out when I have the spare time and report back.
edit: Added in 1a558e8. It looks as though the existing helper ensure_can_construct_http_header_dict already casts when necessary and returns a ValidHTTPHeaderSource.
This allows a simple way to merge headers provided in a request with the ``PoolManager`` headers, rather than replace them.
This allows a simple way to merge headers provided in a request with the
PoolManagerheaders, rather than replace them.Related issue: #2254
I was inspired by Ran's example there:
The basic usage was also tested here. However this approach has limitations - the return type is always a dict, and that happens at the Python level so there isn't much flexibility using that operation if you want to merge a
HTTPHeaderDictinstance either from the pool manager or from the request. The standard dict must have unique keys, so no merging of existing keys is possible, only overwriting.My proposal would allow:
Which can merge, on either side, as expected:
Standard dicts support unions since Python 3.9 (see PEP 584 – Add Union Operators To dict) so I've tried to make the
HTTPHeaderDictquack in that way.