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

add response headers to scope #718

Closed
wants to merge 1 commit into from
Closed

add response headers to scope #718

wants to merge 1 commit into from

Conversation

mindrunner
Copy link

While implementing a custom logger formatter to support gunicorn logging syntax (https://docs.gunicorn.org/en/stable/settings.html#logging), we had some crucial information missing. One of which, we could solve by injecting the response headers to the scope object. This might be a first step towards
#527

@marco-loche-work
Copy link

Please merge it!!!!

@euri10
Copy link
Member

euri10 commented Oct 23, 2020

I don't think the spec has that scope key, am I missing something ?

@mindrunner
Copy link
Author

For me it worked in the specific context I had to use it. However, I am not really into the gunicon/uvicorn code, so I am not entirely sure about the consequences of this change.

@florimondmanca
Copy link
Member

Hello!

I don't think the spec has that scope key, am I missing something ?

Yup, the ASGI spec doesn't define putting response headers into the scope. If that's something you'd be ready to champion in the ASGI repo, then please feel free to. But until then I'd advise us against implementing ASGI-level behavior into Uvicorn that's not defined in the spec — otherwise we break the purpose of ASGI as a standard that enables interoperability between apps and servers. 😕

I'll close this for now, not to say that the original use case isn't appropriate for discussion, but I don't think this is the way we'll be going forward with. If there's any specific use case you can expose for discussion then we can still try and find an alternative solution. :-)

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

Successfully merging this pull request may close these issues.

6 participants