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

Improve wording and structure of pure ASGI middleware docs #1723

Merged
merged 5 commits into from
Aug 9, 2022

Conversation

florimondmanca
Copy link
Member

@florimondmanca florimondmanca commented Jul 1, 2022

This PR is a follow-up based on @Kludex's comment here: #1656 (review)

I went ahead and reviewed the structure of the "Pure ASGI middleware" section, which now looks like this...

  • Pure ASGI middleware
    • Writing pure ASGI middleware
    • Using pure ASGI middleware
    • Type annotations
    • Common patterns
      • Processing certain requests only
      • Sending eager responses
      • Inspecting or modifying the request
      • Inspecting or modifying the response
      • Passing information to endpoints
    • Gotchas
      • ASGI middleware should be stateless

Some other notable changes:

  • Add a section with an example of wrapping receive ("Inspecting or modifying the request").
  • Expand wording to better highlight that WebSocket middleware is just as possible.
  • Add links to ASGI event messages docs when appropriate (http.request, http.response.start, etc). Surely, we know them by heart by now 😄 but we can't assume folks will guess what those contain or where to find that info.
  • Use tabs to add the "Don't vs Do" examples for the "Stateless middleware" gotcha, as suggested by @euri10, which highlights using the nonlocal syntax ("Do") vs storing in attributes ("Don't").
  • Reword the "storing context in scope" based on discussions over at asgiref -- for now, mention this is purely conventional.
  • Add the common pattern of wrapping an app around a try/except/finally block.

(I also tweaked some parts outside of the "Pure ASGI" section, mostly for ensuring consistency; happy to defer to a different PR if that seems out of scope.)

To review, you may view the docs locally, using scripts/docs.

@florimondmanca florimondmanca added the documentation Project documentation label Jul 1, 2022
@florimondmanca florimondmanca requested a review from a team July 1, 2022 20:34
@florimondmanca florimondmanca force-pushed the fm/asgi-middleware-docs-revamp branch 5 times, most recently from 3ba4215 to a380c74 Compare July 1, 2022 21:20
@adriangb
Copy link
Member

adriangb commented Jul 1, 2022

I read over the markdown version. It looks very nice to me, thank you for working on this! I think ASGI middleware is now one of the most comprehensive and well thought out sections of the docs, go team!

@Kludex Kludex added this to the Version 0.21.0 milestone Jul 4, 2022
@euri10
Copy link
Member

euri10 commented Jul 6, 2022

amazing @florimondmanca !

@florimondmanca
Copy link
Member Author

florimondmanca commented Jul 6, 2022

Thanks :-)

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Love the way you write. :)

Added some humble comments 🙏

@@ -226,20 +235,20 @@ Middleware classes should not modify their state outside of the `__init__` metho
Instead you should keep any state local to the `dispatch` method, or pass it
around explicitly, rather than mutating the middleware instance.

!!! warning
Currently, the `BaseHTTPMiddleware` has some known limitations:
### Limitations
Copy link
Member

@Kludex Kludex Jul 6, 2022

Choose a reason for hiding this comment

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

I usually pay more attention to things when I see admonitions. 🤔 Just saying... Feel free to close it. 😗

Copy link
Member Author

@florimondmanca florimondmanca Jul 20, 2022

Choose a reason for hiding this comment

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

The motivation there was to make add those limitations as a section in the navigation, so it's obvious when searching for the "BaseHTTPMiddleware documentation" that "Okay, I can use this, but there are limitations". On the other hand admonitions require scrolling through the actual content to tell.

docs/middleware.md Show resolved Hide resolved
docs/middleware.md Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
docs/middleware.md Outdated Show resolved Hide resolved
Copy link

@iudeen iudeen left a comment

Choose a reason for hiding this comment

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

This doc is so detailed and elaborate! Thanks for this one!

docs/middleware.md Show resolved Hide resolved
Copy link

@iudeen iudeen left a comment

Choose a reason for hiding this comment

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

Can we add some details on custom Responder? (If it makes sense, and is relevant)

feel free to close

docs/middleware.md Outdated Show resolved Hide resolved
@florimondmanca
Copy link
Member Author

florimondmanca commented Jul 20, 2022

Blocked by… tomchristie/mkautodoc#33 (Not that keen to do an equivalent of encode/httpx#2313 for every Encode project that will be affected through using mkautodoc)

@Kludex
Copy link
Member

Kludex commented Jul 21, 2022

Blocked by… tomchristie/mkautodoc#33 (Not that keen to do an equivalent of encode/httpx#2313 for every Encode project that will be affected through using mkautodoc)

There's no hurry for the next release, so I guess it's fine to wait. Did you also ping Tom privately on Gitter about this?

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Thanks @florimondmanca 🙏

@florimondmanca
Copy link
Member Author

Merging since I believe this is ready given the resolution of comments and approvals. Thanks all for reviewing!

@florimondmanca florimondmanca merged commit 5006a30 into master Aug 9, 2022
@florimondmanca florimondmanca deleted the fm/asgi-middleware-docs-revamp branch August 9, 2022 10:08
@gnat
Copy link

gnat commented Aug 19, 2022

Great work @florimondmanca !

Especially the Common Patterns section; I just used it to write my own pure ASGI middleware.

That said, newcomers are undoubtedly still gonna be a bit thrown compared to BaseHTTPMiddleware, but the situation is a lot more manageable with this documentation. We still really should asyncio.shield (or whatever the anyio equivalent is) to fix the BackgroundTask bug.

@JarroVGIT
Copy link

I just wanted to say: this is written very clearly, thank you for the work @florimondmanca !

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

Successfully merging this pull request may close these issues.

7 participants