Exclude the `object` possibility from the `ClientSession.timeout` type #6923

DevilXD · 2022-09-20T13:53:46Z

What do these changes do?

This is a follow-up PR to #6917, which fixes the overly inclusive typing of ClientSession.timeout attribute, which can never actually end up as an object instance. This leftover probably exists from the time when there was the _sentinel value being assigned to the attribute (see "3.8 branch self._timeout assignment" from the linked issue), hence why it ended up this way.

Are there changes in behavior for the user?

For untyped code, there's absolutely no change - the self._timeout attribute was never actually assigned the sentinel value to. Most of the explanation already exists in #6917, here's the test code snippet from there again:

import asyncio
import aiohttp

async def main:
    session = aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=10))
    # read back the total time attribute
    total_time = session.timeout.total  # "object" type of "Union[object, ClientTimeout]" has no attribute "total"
    print(total_time)

asyncio.run(main())

For typed code, the current solution would be to assert isinstance(session.timeout, aiohttp.ClientTimeout) everywhere the attribute is being accessed. This PR removes this "unnecessary necessity".

Related issue number

#6917

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes (documentation never mentioned an object instance being accessible from there even)
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

DevilXD · 2022-09-20T14:02:08Z

I'm not sure if I deserve a CONTRIBUTORS entry for such a simple typing-only change (it specifically asks for "code modification" and this changes nothing other than typing), so I left it out for now (and take no issue with that). If it would be needed though, please let me know and I'll add a commit for it.

webknjaz · 2022-09-20T14:15:20Z

CHANGES/6917.misc

@@ -0,0 +1 @@
+Fix ClientSession.timeout having incorrect typing.


Plz try following the recommendations at https://github.com/aio-libs/aiohttp/tree/master/CHANGES#alright-so-how-to-add-a-news-fragment. Also, use RST formatting to highlight/link identifiers.

Also, this is probably a bugfix + maybe docs type.

I've made the updates myself.

Dreamsorcerer · 2022-09-20T14:36:46Z

I'm not sure if I deserve a CONTRIBUTORS entry...

I've never even added my own name. :P
Not sure that file is really useful, personally

webknjaz · 2022-09-20T15:20:42Z

Merge branch 'master' into patch-1

@Dreamsorcerer FYI you can now select a rebase mode in that GH button (just click on the drop-down arrow)

codecov · 2022-09-20T15:23:20Z

Codecov Report

Merging #6923 (d85813c) into master (20f0531) will not change coverage.
The diff coverage is 100.00%.

❗ Current head d85813c differs from pull request most recent head 9b77aa4. Consider uploading reports for the commit 9b77aa4 to get more accurate results

@@           Coverage Diff           @@
##           master    #6923   +/-   ##
=======================================
  Coverage   97.35%   97.35%           
=======================================
  Files         106      106           
  Lines       30962    30962           
  Branches     3843     3843           
=======================================
  Hits        30144    30144           
  Misses        619      619           
  Partials      199      199

Flag	Coverage Δ
CI-GHA	`97.26% <100.00%> (ø)`
OS-Linux	`96.91% <100.00%> (-0.01%)`	⬇️
OS-Windows	`95.37% <ø> (-0.01%)`	⬇️
OS-macOS	`96.54% <100.00%> (ø)`
Py-3.10.6	`96.67% <100.00%> (+<0.01%)`	⬆️
Py-3.10.7	`96.80% <100.00%> (+<0.01%)`	⬆️
Py-3.11.0-rc.2	`96.44% <100.00%> (ø)`
Py-3.7.13	`96.80% <100.00%> (+0.05%)`	⬆️
Py-3.7.9	`95.26% <ø> (ø)`
Py-3.8.10	`95.15% <ø> (ø)`
Py-3.8.13	`96.56% <100.00%> (-0.09%)`	⬇️
Py-3.8.14	`96.52% <100.00%> (?)`
Py-3.9.13	`96.97% <100.00%> (+0.03%)`	⬆️
Py-3.9.14	`96.54% <100.00%> (ø)`
Py-pypy7.3.9	`96.28% <100.00%> (-0.01%)`	⬇️
VM-macos	`96.54% <100.00%> (ø)`
VM-ubuntu	`96.91% <100.00%> (-0.01%)`	⬇️
VM-windows	`95.37% <ø> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
aiohttp/client.py	`94.36% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Dreamsorcerer · 2022-09-20T15:40:35Z

@Dreamsorcerer FYI you can now select a rebase mode in that GH button (just click on the drop-down arrow)

Ah, it all get squash committed anyway though, so I'm not it makes any difference.

Dreamsorcerer

This seems obviously correct to me, probably didn't need the long story in the bug ticket. :P

Dreamsorcerer · 2022-09-20T15:43:47Z

@webknjaz Seems like a simple fix for 3.8.2 if we've not already passed the point of no return?

webknjaz · 2022-09-20T16:08:23Z

This seems obviously correct to me, probably didn't need the long story in the bug ticket. :P

A good description is useful, since what's obvious for one person would be confusing for another. Especially, this applies to a “future you”, or a “trying to decide on backports you”, or a “Git archeologist-you”.

webknjaz · 2022-09-20T16:14:08Z

@webknjaz Seems like a simple fix for 3.8.2 if we've not already passed the point of no return?

I guess I could get it in. But it still needs brushing up the change note.

patchback · 2022-09-20T16:48:56Z

Backport to 3.9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.9/78fa04019c49e87f352d89e000963c16af78cae1/pr-6923

Backported as #6924

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

This is a follow-up PR to #6917, which fixes the overly inclusive typing of `ClientSession.timeout` attribute, which can never actually end up as an `object` instance. This leftover probably exists from the time when there was the `_sentinel` value being assigned to the attribute (see "3.8 branch `self._timeout` assignment" from the linked issue), hence why it ended up this way. For untyped code, there's absolutely no change — the `self._timeout` attribute was never actually assigned the sentinel value to. Most of the explanation already exists in #6917, here's the test code snippet from there again: ```py import asyncio import aiohttp async def main: session = aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=10)) # read back the total time attribute total_time = session.timeout.total # "object" type of "Union[object, ClientTimeout]" has no attribute "total" print(total_time) asyncio.run(main()) ``` For typed code, the current solution would be to `assert isinstance(session.timeout, aiohttp.ClientTimeout)` everywhere the attribute is being accessed. This PR removes this "unnecessary necessity". Co-authored-by: Sviatoslav Sydorenko <wk@sydorenko.org.ua> Fixes #6917 PR #6923 (cherry picked from commit 78fa040)

patchback · 2022-09-20T16:49:07Z

Backport to 3.8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.8/78fa04019c49e87f352d89e000963c16af78cae1/pr-6923

Backported as #6925

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

This is a follow-up PR to #6917, which fixes the overly inclusive typing of `ClientSession.timeout` attribute, which can never actually end up as an `object` instance. This leftover probably exists from the time when there was the `_sentinel` value being assigned to the attribute (see "3.8 branch `self._timeout` assignment" from the linked issue), hence why it ended up this way. For untyped code, there's absolutely no change — the `self._timeout` attribute was never actually assigned the sentinel value to. Most of the explanation already exists in #6917, here's the test code snippet from there again: ```py import asyncio import aiohttp async def main: session = aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=10)) # read back the total time attribute total_time = session.timeout.total # "object" type of "Union[object, ClientTimeout]" has no attribute "total" print(total_time) asyncio.run(main()) ``` For typed code, the current solution would be to `assert isinstance(session.timeout, aiohttp.ClientTimeout)` everywhere the attribute is being accessed. This PR removes this "unnecessary necessity". Co-authored-by: Sviatoslav Sydorenko <wk@sydorenko.org.ua> Fixes #6917 PR #6923 (cherry picked from commit 78fa040)

…om the `ClientSession.timeout` type (#6924) **This is a backport of PR #6923 as merged into master (78fa040).**  ## What do these changes do?  This is a follow-up PR to #6917, which fixes the overly inclusive typing of `ClientSession.timeout` attribute, which can never actually end up as an `object` instance. This leftover probably exists from the time when there was the `_sentinel` value being assigned to the attribute (see "3.8 branch `self._timeout` assignment" from the linked issue), hence why it ended up this way. ## Are there changes in behavior for the user?  For untyped code, there's absolutely no change - the `self._timeout` attribute was never actually assigned the sentinel value to. Most of the explanation already exists in #6917, here's the test code snippet from there again: ```py import asyncio import aiohttp async def main: session = aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=10)) # read back the total time attribute total_time = session.timeout.total # "object" type of "Union[object, ClientTimeout]" has no attribute "total" print(total_time) asyncio.run(main()) ``` For typed code, the current solution would be to `assert isinstance(session.timeout, aiohttp.ClientTimeout)` everywhere the attribute is being accessed. This PR removes this "unnecessary necessity". ## Related issue number  #6917 ## Checklist - [x] I think the code is well written - [ ] Unit tests for the changes exist - [x] Documentation reflects the changes (documentation never mentioned an `object` instance being accessible from there even) - [ ] If you provide code modification, please add yourself to `CONTRIBUTORS.txt` * The format is <Name> <Surname>. * Please keep alphabetical order, the file is sorted by names. - [ ] Add a new news fragment into the `CHANGES` folder * name it `<issue_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." Co-authored-by: DevilXD <DevilXD@users.noreply.github.com>

…om the `ClientSession.timeout` type (#6925) **This is a backport of PR #6923 as merged into master (78fa040).**  ## What do these changes do?  This is a follow-up PR to #6917, which fixes the overly inclusive typing of `ClientSession.timeout` attribute, which can never actually end up as an `object` instance. This leftover probably exists from the time when there was the `_sentinel` value being assigned to the attribute (see "3.8 branch `self._timeout` assignment" from the linked issue), hence why it ended up this way. ## Are there changes in behavior for the user?  For untyped code, there's absolutely no change - the `self._timeout` attribute was never actually assigned the sentinel value to. Most of the explanation already exists in #6917, here's the test code snippet from there again: ```py import asyncio import aiohttp async def main: session = aiohttp.ClientSession(timeout=aiohttp.ClientTimeout(total=10)) # read back the total time attribute total_time = session.timeout.total # "object" type of "Union[object, ClientTimeout]" has no attribute "total" print(total_time) asyncio.run(main()) ``` For typed code, the current solution would be to `assert isinstance(session.timeout, aiohttp.ClientTimeout)` everywhere the attribute is being accessed. This PR removes this "unnecessary necessity". ## Related issue number  #6917 ## Checklist - [x] I think the code is well written - [ ] Unit tests for the changes exist - [x] Documentation reflects the changes (documentation never mentioned an `object` instance being accessible from there even) - [ ] If you provide code modification, please add yourself to `CONTRIBUTORS.txt` * The format is <Name> <Surname>. * Please keep alphabetical order, the file is sorted by names. - [ ] Add a new news fragment into the `CHANGES` folder * name it `<issue_id>.<type>` for example (588.bugfix) * if you don't have an `issue_id` change it to the pr id after creating the pr * ensure type is one of the following: * `.feature`: Signifying a new feature. * `.bugfix`: Signifying a bug fix. * `.doc`: Signifying a documentation improvement. * `.removal`: Signifying a deprecation or removal of public API. * `.misc`: A ticket has been closed, but it is not of interest to users. * Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files." Co-authored-by: DevilXD <DevilXD@users.noreply.github.com>

DevilXD requested a review from asvetlov as a code owner September 20, 2022 13:53

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Sep 20, 2022

DevilXD mentioned this pull request Sep 20, 2022

ClientSession.timeout has an incorrect typing #6917

Closed

1 task

webknjaz requested a review from Dreamsorcerer September 20, 2022 14:13

webknjaz reviewed Sep 20, 2022

View reviewed changes

Dreamsorcerer approved these changes Sep 20, 2022

View reviewed changes

Dreamsorcerer added the backport-3.9 Trigger automatic backporting to the 3.9 release branch by Patchback robot label Sep 20, 2022

DevilXD added 2 commits September 20, 2022 18:16

Fix ClientSession.timeout typing

5411937

Add a news fragment

31905a4

webknjaz added a commit to DevilXD/aiohttp that referenced this pull request Sep 20, 2022

Link the change notes to PR aio-libs#6923

6c09879

webknjaz force-pushed the patch-1 branch from d85813c to 6c09879 Compare September 20, 2022 16:39

webknjaz changed the title ~~Fix ClientSession.timeout having incorrect typing~~ Exclude the object possibility from the ClientSession.timeout type Sep 20, 2022

webknjaz added 4 commits September 20, 2022 18:44

Update the change note to reflect the type change

229b73d

Switch the change note type to "bugfix"

3be3fb9

Alias the change not to the "doc" category too

fee3de1

Link the change notes to PR aio-libs#6923

9b77aa4

webknjaz force-pushed the patch-1 branch from 6c09879 to 9b77aa4 Compare September 20, 2022 16:44

webknjaz merged commit 78fa040 into aio-libs:master Sep 20, 2022

webknjaz added the backport-3.8 label Sep 20, 2022

patchback bot mentioned this pull request Sep 20, 2022

[PR #6923/78fa0401 backport][3.9] Exclude the object possibility from the ClientSession.timeout type #6924

Merged

5 tasks

patchback bot mentioned this pull request Sep 20, 2022

[PR #6923/78fa0401 backport][3.8] Exclude the object possibility from the ClientSession.timeout type #6925

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exclude the `object` possibility from the `ClientSession.timeout` type #6923

Exclude the `object` possibility from the `ClientSession.timeout` type #6923

DevilXD commented Sep 20, 2022 •

edited

DevilXD commented Sep 20, 2022

webknjaz Sep 20, 2022

webknjaz Sep 20, 2022

webknjaz Sep 20, 2022

Dreamsorcerer commented Sep 20, 2022

webknjaz commented Sep 20, 2022

codecov bot commented Sep 20, 2022 •

edited

Dreamsorcerer commented Sep 20, 2022

Dreamsorcerer left a comment

Dreamsorcerer commented Sep 20, 2022

webknjaz commented Sep 20, 2022

webknjaz commented Sep 20, 2022

patchback bot commented Sep 20, 2022 •

edited

patchback bot commented Sep 20, 2022 •

edited

		@@ -0,0 +1 @@
		Fix ClientSession.timeout having incorrect typing.

Exclude the object possibility from the ClientSession.timeout type #6923

Exclude the object possibility from the ClientSession.timeout type #6923

Conversation

DevilXD commented Sep 20, 2022 • edited

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

DevilXD commented Sep 20, 2022

webknjaz Sep 20, 2022

Choose a reason for hiding this comment

webknjaz Sep 20, 2022

Choose a reason for hiding this comment

webknjaz Sep 20, 2022

Choose a reason for hiding this comment

Dreamsorcerer commented Sep 20, 2022

webknjaz commented Sep 20, 2022

codecov bot commented Sep 20, 2022 • edited

Codecov Report

Dreamsorcerer commented Sep 20, 2022

Dreamsorcerer left a comment

Choose a reason for hiding this comment

Dreamsorcerer commented Sep 20, 2022

webknjaz commented Sep 20, 2022

webknjaz commented Sep 20, 2022

patchback bot commented Sep 20, 2022 • edited

Backport to 3.9: 💚 backport PR created

patchback bot commented Sep 20, 2022 • edited

Backport to 3.8: 💚 backport PR created

Exclude the `object` possibility from the `ClientSession.timeout` type #6923

Exclude the `object` possibility from the `ClientSession.timeout` type #6923

DevilXD commented Sep 20, 2022 •

edited

codecov bot commented Sep 20, 2022 •

edited

patchback bot commented Sep 20, 2022 •

edited

patchback bot commented Sep 20, 2022 •

edited