altered internal loops of how ``SessionRedirectMixin.resolve_redirect…

[jvanasco]

This is an idea for updating the resolve_redirects loop.

The existing code was hard to untangle as variables get overwritten midway through the loop (resp switches between the previous and the next, which can be easy to miss). It manages the hist by omitting the first request, appending the previous request, then shuffling some stuff around in a cleanup during the send method.

In this approach, populating the hist is moved down the loop so the current request is topmost on the stack. The cleanup function is then simplified to just popping it off.

This passes all tests, but causes a slight undocumented API change -- the value of request.history changes, requiring a slightly different cleanup. The only code that calls resolve_redirects is within Session.send, but I don't know if this would break any hooks.

anyways, I think this makes the block more straightforward as the "cleanup" function in send is simplified and the context of resp when appending the history doesn't change as much. another approach I thought of was using inline comments or variable name changes to clearly note which is the "previous_request" in that loop.

[codecov-io]

Codecov Report

Merging #3871 into master will decrease coverage by -0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #3871     +/-   ##
=========================================
- Coverage   89.17%   89.07%   -0.1%     
=========================================
  Files          15       15             
  Lines        1893     1894      +1     
=========================================
- Hits         1688     1687      -1     
- Misses        205      207      +2

Impacted Files	Coverage Δ
requests/sessions.py	92.88% <100%> (-0.06%)	❌
requests/models.py	93.42% <ø> (-0.46%)	❌
requests/utils.py	84.94% <ø> (+0.12%)	✅

---

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 90f3842...d49e839. Read the comment docs.

[Lukasa]

So I agree that this is a clarity improvement, but I am worried about the minor API changes. These are places that get overridden and subclassed a lot, and I have a real suspicion we'll break some stuff. I wonder if we should move this to the 3.0 branch instead.

[Lukasa]

No need for the trailing comma in the list.

[jvanasco]

3.0 sounds good to me. I can remove the trailing comma. flake8 yells at me when i leave it out ;)

[sigmavirus24]

flake8 yells at me when i leave it out ;)

What flake8 plugins are you using? That is not default behaviour. (Also, side note, Kenneth hates Flake8 with a passion ;))

[jvanasco]

updated, however I should probably do a different pr against the 3.0 branch instead -- right?

What flake8 plugins are you using? That is not default behaviour.

flake8-commas, but installed after years of enforcing it as a style guide, after years of having it enforced on me as a style guide. I have a love-hate relationship with flake8, my exceptions list is long, and there are a lot of situations that pyflakes doesn't cover... but it's become really useful as a sanity-check before running unit tests.

[Lukasa]

Yeah, you'd have to do a different PR against the 3.0 branch.

[jvanasco]

closed in favor of #3886 for the 3.0 branch