Skip to content

Conversation

@evilensky
Copy link
Contributor

@evilensky evilensky commented Dec 21, 2022

Resolves #9390.

The session.headers may be set to any mapping according to the project, and mutable mapping appears to be the most correct annotation.

@github-actions

This comment has been minimized.

@evilensky evilensky force-pushed the FIX-session-headers-mapping-type branch from 2c74b5a to 5528de7 Compare December 21, 2022 16:35
@github-actions

This comment has been minimized.

@evilensky evilensky changed the title set session.headers to same type as request.headers set session.headers to MutableMapping Dec 21, 2022
headers: CaseInsensitiveDict[str | bytes]
# See https://github.com/psf/requests/issues/5020#issuecomment-989082461:
# requests sets this as a CaseInsensitiveDict, but users may set it to any MutableMapping
headers: MutableMapping[str, str | bytes | None]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this should be MutableMapping[str, str | bytes | None] rather than MutableMapping[str, str | bytes]? This annotation was debated somewhat extensively in #7773 (comment). @janrito said at the time:

We want to allow str or bytes on actual headers, but None only on updates

Copy link
Contributor Author

@evilensky evilensky Dec 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I am not sure. But given that this code does not produce any runtime error:

session = requests.Session()
session.headers = None
resp = session.get('http://example.com')
assert resp.status_code == 200

It appears headers get merged-or-set-to-defaults with None? https://github.com/psf/requests/blob/main/requests/sessions.py#L61-L71

Edit for clarity: I'm open to any guidance on whether type checking should reflect runtime behavior or do "something else."

Copy link
Member

@AlexWaygood AlexWaygood Dec 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's okay to set session.headers to None, then the annotation should be headers: MutableMapping[str, str | bytes] | None rather than MutableMapping[str, str | bytes | None]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the mypy_primer output, looks like changing it to MutableMapping[str, str | bytes] | None would be quite disruptive -- so I vote for keeping this as MutableMapping[str, str | bytes] for now, unless keeping it that way is causing a concrete problem for you.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @evilensky! Will merge once the CI has finished.

requests is tricky to write stubs for, there's loads of unexpected things that crop up...

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood AlexWaygood merged commit 6a7839f into python:main Dec 22, 2022
@evilensky evilensky deleted the FIX-session-headers-mapping-type branch December 22, 2022 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

requests.session().headers

2 participants