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

Clarify general and spec documentation #83

Merged
merged 5 commits into from
Apr 22, 2019
Merged

Clarify general and spec documentation #83

merged 5 commits into from
Apr 22, 2019

Conversation

daneah
Copy link
Contributor

@daneah daneah commented Apr 20, 2019

Hey folks, I'm keen on making myself useful toward getting ASGI into Django (I talked with @andrewgodwin at DjangoCon US 2018 about it and recently saw @tomchristie's DjangoCon EU 2019 keynote) and I'm starting pretty well from scratch by reading the ASGI specification docs. Since I've just gone through the thing, I wanted to contribute some clarifying changes to things that tripped me up that I hope will provide some value to others.

The biggest change was moving data types for protocol specifications to a consistent place for better readability, e.g.:

Before

  • type: http
  • asgi["version"]: The version of the ASGI spec, as a string

After

  • type (Unicode string) – "http"
  • asgi["version"] (Unicode string) – Version of the ASGI spec

The rest being mostly minor stuff for consistency:

  • Fix a few typographical errors, punctuation, etc.
  • Rephrase a few passages for clarity
  • Use consistent capitalization of "Unicode"
  • Use consistent phrasing for "percent encoding"
  • Change double-negative "not decoded"/"not unencoded" to "encoded"

Hope this helps; I'll be reading more on Starlette, uvicorn, etc. as well as Django roadmap stuff where I can, and if there are any specific areas you need help with presently please don't hesitate to let me know. Cheers!

* Fix a few typographical errors, punctuation, etc.
* Make data types in keys specifications for protocols more scannable by
moving them to a reliable location
* Rephrase a few passages for clarity
@daneah
Copy link
Contributor Author

daneah commented Apr 20, 2019

Incidentally I had to manually install sphinx and comment out some stuff in docs/conf.py to get the docs building locally; is there a pre-baked something that I'm missing around that?

Copy link
Member

@andrewgodwin andrewgodwin left a comment

Choose a reason for hiding this comment

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

I like the clarifications and copy-editing overall, but I'd like a few fixes to style (no fancy dashes, and some other things called out in comments)

As for building the docs - there's no single-step thing to get that working; a document would be nice. I don't think it's worth scripting as it is just "install Sphinx and it should work".

coroutine callable. This second callable gets given ``send`` and ``receive``
awaitables that allow the coroutine to monitor incoming events and send
outgoing events.
ASGI is structured as a double-callable---the first callable takes a
Copy link
Member

Choose a reason for hiding this comment

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

Not a huge fan of en or em dashes, they have confusing subtlety for those unfamiliar with them. Let's keep the normal dashes, and use semicolons or periods if a bigger context break is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me; this specific line is what put me on that path since "double-callable - the first" was visually busy. I'll back most of these dash changes out and replace with sentence breaks where it makes sense!

``scope``, which contains details about the incoming request, and returns a
second, coroutine callable. This second callable is given ``send`` and
``receive`` awaitables that allow the coroutine to monitor incoming events and
send outgoing events.
Copy link
Member

Choose a reason for hiding this comment

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

I've just realised I need to revise this part of the intro for the new spec. I'll do that once this lands.

specs/asgi.rst Outdated
with an Exception (of any type). Failure to do this may result in the server
thinking you support a protocol you don't, which can be confusing with
the Lifespan protocol, as the server will wait to start until you tell it.
with an exception (of any type). Failure to do this may result in the server
Copy link
Member

Choose a reason for hiding this comment

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

Exception is actually the right capitalisation here as it's the name of the root Python type that is expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had flipped back and forth on this one; perhaps stylizing as Exception vs. Exception is a good compromise there 👍

Copy link
Member

Choose a reason for hiding this comment

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

I agree with that.

specs/asgi.rst Outdated Show resolved Hide resolved
specs/asgi.rst Outdated

As an example, imagine a HTTP protocol server wishes to provide an extension
As an example, imagine an HTTP protocol server wishes to provide an extension
Copy link
Member

Choose a reason for hiding this comment

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

I would not choose to put an before a strong H sound, like in HTTP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, this may be a British/American English difference I wasn't familiar with 😉I tried to avoid those on the whole, happy to back this out!

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it's probably the aitch versus haitch pronunciation thing. While Django doc standard is US English, I would like to leave it as a, just for me :)

@daneah
Copy link
Contributor Author

daneah commented Apr 21, 2019

Thanks for the feedback @andrewgodwin; those points should all be addressed now! The error I was seeing after installing sphinx and running make html in docs/ is the following:

...
Theme error:
An error happened in rendering the page extensions.
Reason: TemplateNotFound()
make: *** [html] Error 2

That led me here, which worked. Maybe installing the theme is what I was missing 🤔

@andrewgodwin
Copy link
Member

Thanks! I'll merge it in.

As for the theme, I'm not sure why I would have that theme and a fresh install wouldn't. Maybe it's a package version thing?

@andrewgodwin andrewgodwin merged commit 90535f2 into django:master Apr 22, 2019
@daneah
Copy link
Contributor Author

daneah commented Apr 22, 2019

Yeah, possible! If I'm tinkering later I'll see if I can suss out what might be implicated there.

@daneah daneah mentioned this pull request Apr 25, 2019
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.

2 participants