Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Document how to create ASGI middlewares #1656
Document how to create ASGI middlewares #1656
Changes from 23 commits
09ce984
7c51da5
7975168
a51d5c1
ea082ab
9672c0d
961b98d
7aa5575
a4800fc
6f9eda4
3384cb1
c6568bd
46662e9
6948b81
c737fb1
9a3e9e8
b0f8029
3dd6b11
514c818
5150507
0408af2
5b54dd9
aa6bbae
4f12d08
efbf115
d218689
a2c504c
74664e1
77e2c27
b2af992
046f4d4
263de76
8868e49
756036b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this more explicit assignment is clear than relying on
MutableHeaders
to modifymessage
in-place.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementations I've seen use it in the way I'm using it. I do agree with you on which one is clear, but I'd rather prefer the user to think a bit on a small documentation like this instead of another more complicated code source.
That being said, if you are strong about it, we can add a note about it below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't say I feel strongly about it, but it is 1 LOC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also there’s a typo here: for key, value in self.headers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option which I personally like is using a generator:
Maybe it's worth mentioning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s wild!
Also, hmm… I’m less convinced about documenting the « responder class » now, although I’m aware I’m the one who brought it up initially. That’s because that’s a style I’ve been using. I think I was itches by the fact we’d be redefining a function every time with the closure style.
There’s a couple of styles that could be used, eg. responder class, or this generator style, etc. I think this section is complex and new enough for most readers already that we should stick to just one.
I’m curious what you would think about dropping this section. We’ve got a note somewhere on the fact that middleware classes should be stateless. The closure style actually doesn’t incite users to use class attributes, so it doesn’t pose a « per request state » problem, so we could drop this entire section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a neat pattern huh! Generators are great little state machines that you can ratchet forwards and pass messages to. And of course you can go full functional:
I do think this section is getting a bit in depth, maybe at this point we should let users fly and use their imagination and dev skills to figure out which solution they want to use? We'll still need to explain why they should not use attributes on the middleware. And maybe the best way to do that is with a counterexample, and if I had to pick one style I'd pick the responder class because I think it'll be easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to re read this section several times, I think it would be extremely clear to have a
don't do this example
and another one following,do this instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to simplify this section and apply @euri10 's comment. I may remove the snippets here, and point to external implementations...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can even point to our own internal implementations? GZipMiddleware would be a good example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hehe, I always struggle with generators. 😅 What I don't like about
yield
for receiving stuff is that there's no way to have type inference, any values obtained at the left side would have to be manually annotated to have typing support (autocompletion, inline errors, mypy, etc). I'm also not sure what better approach is there. But anyway...I would leave this PR without more examples, to try and limit the scope. Maybe a future PR could include suggesting more examples, but I think it would be good to have this close to what is already there, without adding too much.
I would just add another phrase clarifying the stateless stuff. I'm adding that as a separate suggestion below. 🤓