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
Add Mount(..., middleware=[...]) #1649
Add Mount(..., middleware=[...]) #1649
Changes from 27 commits
42548cf
01bbcdc
d8b626d
73dc39e
14d3005
8eb2699
baab334
bbca389
a75a523
578f618
ba59a35
3cadfb2
1eace2b
ca50340
f65dfc8
5edb100
1ef66e6
fb93ef5
273cc73
4369ee7
10f47ae
feeba5e
0bb54e4
d7c3f2a
e6fad81
adb52ab
0f7a2c4
f6de20f
72fbaa6
eee6a6f
aec580f
93ec37f
5f936e9
0079756
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.
Hmm... This works... 🤔
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.
Try this example:
The "on_mount" middleware will never be called because PotatoException tears through
Mount
's middleware stack. On the other hand "on_app" always gets called because it's "protected" by ErrorMiddleware.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.
Ah, got it. But that's not what I got from what's written here...
Do you think an image with middleware/app/mount as "blocks" would be helpful understanding this? 🤔
(I can help if you think it makes sense, but I lack design skills to do it 😎 👍)
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 sounds like a good idea, please give it a shot! I won't have time until next week (and also don't have the design skills 😆). Maybe do it with ascii text instead of an image since it will be easier to embed?
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.
This is not a blocker. 👍
I'll not have time to do this, if someone eventually wants, feel free to open a PR with it. 👍
(Please do not resolve this conversation so ppl see it)
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.
Yes, that's right.
Adrian's example was missing something (I had to add it myself before, and I forgot)...
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.
We can use @florimondmanca 's drawing here for the explanation 👍
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.
Sorry for the broken example 😫, but yes you got it right Florimond!
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 added a test in aec580f
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.
@florimondmanca I'm realizing now that it was not clear if this was completely resolved, sorry if I missed it. Do you think we should tweak the docs more, maybe adding something along the lines of your explanation in #1649 (comment)?
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.
Should we also
assert
that calling a route not under theMount
does not apply the middleware? E.g. testing atest_client.get("/")
.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.
Added in eee6a6f
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.
Duplicated as well, so I think we can get rid of the
@parametrize
.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.
Fixed in eee6a6f