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

rewrite_response hook: (HTTP status) code updates should sometimes automatically update reason #304

Merged
merged 3 commits into from
Nov 29, 2021

Conversation

maresb
Copy link
Contributor

@maresb maresb commented Nov 24, 2021

I noticed an annoying discrepancy between Tornado's HTTPResponse and my RewritableResponse when changing the HTTP code. In Tornado, the .reason attribute defaults to the text corresponding to the .code attribute. However, in RewritableResponse, we expect the .code attribute to be changed. If you change .code, then .reason retains the text generated for the previous code. In this PR, I add an observer which updates the .reason attribute if .code has changed and .reason still has its default value.

I also extended the documentation to illustrate more of the functionality provided, and extended the tests accordingly.

Co-authored-by: Erik Sundell <erik.i.sundell@gmail.com>
@maresb
Copy link
Contributor Author

maresb commented Nov 29, 2021

Thanks @consideRatio, your comment is very nicely written.

@consideRatio consideRatio merged commit e82bb55 into jupyterhub:master Nov 29, 2021
@consideRatio consideRatio changed the title Set reason according to response code rewrite_response hook: (HTTP status) code updates should sometimes automatically update reason Nov 29, 2021
@maresb maresb deleted the fix-reason branch November 29, 2021 10:55
@meeseeksmachine
Copy link

This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there:

https://discourse.jupyter.org/t/a-template-repository-for-running-latest-rstudio-r-on-mybinder-org-your-jupyterhub/12235/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants