implement response check

[samuelcolvin]

What do these changes do?

Add response_check kwarg to ClientSession for customised raise_for_status checks.

This is just a proof of concept, not complete yet.

Could make response_check completely independent of raise_for_status and remove the second argument from response_check, alternatively could make raise_for_status optionally a function/coroutine instead of adding response_check?

I've switched to allowing raise_for_status as a coroutine, I think this is cleaner and easier to understand.

Are there changes in behavior for the user?

This is backwards compatible

Related issue number

#3864

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• 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."

[codecov-io]

Codecov Report

Merging #3892 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3892      +/-   ##
==========================================
- Coverage    97.8%   97.75%   -0.06%     
==========================================
  Files          43       43              
  Lines        8763     8775      +12     
  Branches     1375     1377       +2     
==========================================
+ Hits         8571     8578       +7     
- Misses         80       83       +3     
- Partials      112      114       +2

Impacted Files	Coverage Δ
aiohttp/web_fileresponse.py	96.59% <0%> (-1.14%)	⬇️
aiohttp/web_protocol.py	91.59% <0%> (-0.77%)	⬇️
aiohttp/web_urldispatcher.py	99.39% <0%> (ø)	⬆️
aiohttp/test_utils.py	99.67% <0%> (ø)	⬆️
aiohttp/connector.py	97.24% <0%> (ø)	⬆️
aiohttp/http_parser.py	97.1% <0%> (+0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e804981...c8eded8. Read the comment docs.

[samuelcolvin]

happy to fix, but please decide if you're happy with the feature in general before I finish the PR.

[webknjaz]

I think I'm fine with the feature in general. This will require a documentation update and a changelog fragment submitted. But I'd also ask @asvetlov what he thinks.

[asvetlov]

@samuelcolvin would you finish the PR?
I don't your plans but documentation and change note are missing.

[samuelcolvin]

Changes made based on reviews.

@asvetlov as far as I'm concerned this is ready. Sorry for the slow response.

[asvetlov]

There is a subtle regression here.
Before a user was allowed to pass 1 / 0 or even an object with __bool__() dunder method implemented.
We discourage this by providing bool type hint but still support.
Now the check works by explicit is True comparison.
I rather suggest reorder checks:

if raise_for_status is None:
    pass
elif callable(raise_for_status):
    await raise_for_status(resp)
elif raise_for_status:  # a shortcut for `bool(raise_for_status)`
    resp.raise_for_status()

What do you think?

[samuelcolvin]

done

[asvetlov]

LGTM, please merge

[HansBrende]

@asvetlov Any reason we wouldn't be able to get this in a stable 3.X release? It seems to be fully backwards compatible... dying to use this feature for several years now. 😞

[patchback]

Backport to 3.9: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply e5beaca on top of patchback/backports/3.9/e5beaca052434cbf1c1814170917085eb271ba43/pr-3892

Backporting merged PR #3892 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.9/e5beaca052434cbf1c1814170917085eb271ba43/pr-3892 upstream/3.9

4. Now, cherry-pick PR implement response check #3892 contents into that branch:

   $ git cherry-pick -x e5beaca052434cbf1c1814170917085eb271ba43

   If it'll yell at you with something like fatal: Commit e5beaca052434cbf1c1814170917085eb271ba43 is a merge but no -m option was given., add -m 1 as follows intead:

   $ git cherry-pick -m1 -x e5beaca052434cbf1c1814170917085eb271ba43

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR implement response check #3892 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.9/e5beaca052434cbf1c1814170917085eb271ba43/pr-3892

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

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

[Dreamsorcerer]

Looks OK, but a 3 year old PR is not going to backport cleanly. Feel free to create a backport PR, ideally following the patchback instructions above.

[HansBrende]

@samuelcolvin any chance you'd be willing to do this backport to 3.9?

[Dreamsorcerer]

He has several other projects to maintain, I doubt you'll get a response. If you want it backported, you'll have to do it yourself.

[HansBrende]

@Dreamsorcerer it has been over a year since this was patchbacked to 3.9 (and 4 years since it was initially merged), yet no sign of a 3.9 release... is that coming soonish or do we also need to patchback to 3.8 to get it within, say, a decade of the initial merge?