Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass response to TooManyRedirectsException #2660

Merged
merged 2 commits into from
Sep 30, 2020

Conversation

davidgrayston
Copy link
Contributor

Fixed

  • TooManyRedirectsException now has response

Note: #2589 includes additional breaking changes to ensure RequestException always has a response, which will be considered for version 8.0.0

If you'd prefer this fix to remain as part of #2589 I'll close this PR

Copy link
Member

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

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

This PR extracts a non-breaking change from #2589, for 7.x. 👍

@gmponos
Copy link
Member

gmponos commented Jun 13, 2020

Hi! I saw the comment from @Tobion ..

Looks like TooManyRedirectsException does not have a response but should have.

but I wonder.. when you have too-many-redirects.. it most probably mean you also have many-responses.. so which response out of these should be included to the exception? Why only the last one?

@davidgrayston
Copy link
Contributor Author

davidgrayston commented Jun 13, 2020

The main reason I used the last response is because that's the one available when the exception is thrown. I suppose it gives some context to what's happening, but perhaps it isn't providing much value?

If this causes more confusion than it is helpful, and there's a decision not to make the response mandatory, perhaps this idea should be abandoned? (happy to close both PRs if that's the case?)

@gmponos
Copy link
Member

gmponos commented Jun 13, 2020

I am open to the discussion as well.. let's leave this open.. It's just the reason why I am not 100% sure about this.

@Nyholm Nyholm added this to the 7.2.0 milestone Sep 22, 2020
@Nyholm Nyholm added Review wanted lifecycle/keep-open Issues that should not be closed labels Sep 22, 2020
@Nyholm
Copy link
Member

Nyholm commented Sep 30, 2020

What is the status of this PR?

@GrahamCampbell
Copy link
Member

I think it just needs to be rebased on master?

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

I think this looks good. Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/keep-open Issues that should not be closed Review wanted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants