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

Migrate docs to Sphinx + MyST #285

Merged
merged 14 commits into from Apr 9, 2021
Merged

Migrate docs to Sphinx + MyST #285

merged 14 commits into from Apr 9, 2021

Conversation

florimondmanca
Copy link
Member

@florimondmanca florimondmanca commented Mar 21, 2021

Refs encode/httpx#1220

There are pending discussions on "MkDocs vs Sphinx" for HTTPX.

This PR goes ahead and experiments with migrating to modern Sphinx for HTTPCore.

This is a fully functional PR. I find the result so much better from a reader experience perspective that I went ahead and deployed, see https://www.encode.io/httpcore/.

Preview

Capture d’écran 2021-03-21 à 15 34 40

Pieces

  • Sphinx
  • sphinx-autobuild: "watch" behavior for the scripts/docs serve script.
  • sphinx.ext.autodoc: API autodoc
  • sphinx.ext.viewcode: automatic [source] links in API reference and source pages
  • MyST: Markdown support for Sphinx — allows us to keep writing docs in Markdown.
  • Furo (cc @pradyunsg): theming (inspired by mkdocs-material and gitbooks). Also seems to support light/dark theming?
  • ghp-import: GitHub Pages deployment (used by MkDocs too)

Some resources I used to help me set things up:

  • https://noumenal.es/notes/sphinx/ - Carlton Gibson's notes on Sphinx + MyST.
  • MyST docs - They contain helpful "how-to" guides and hints for setting up and using eg autodoc and cross-references.
  • Lots of web searching to figure out the specifics of autodoc syntax / viewcode / etc. :-)

Highlights

We now get the following:

  • Cross-references (Sphinx "interlinks") to code items (classes, methods, etc) from prose, function signatures, etc. Just look at https://www.encode.io/httpcore/api.html — the API reference is so much more useful and usable as a result.
  • All of Sphinx's default niceties: zero-config syntax highlighting, source links, built-in search index, etc.

What changed in the development workflow:

  • Docstrings: switched from our ad-hoc style to the NumPy style. I chose it because it is visually very close to our initial style, while still being easy to read (imo). The Sphinx napoleon extension parses these docstrings to generate the corresponding output.
    • Caveat: prose in docstrings is reST, not Markdown. So inline code should be written as ''code'' (double backticks), rather than 'code' (single backtick), and code fences should be written as indented code preceded by a line ending with :: (see Example:: in _bytestreams.py).
  • Docs prose: instead of mkautodoc blocks such as ::: mkautodoc, we now have to use MyST-Parser special :::{directive} style combined with Sphinx directive contents inside it. It's a bit odd at first, but it makes sense. MyST supports reStructuredText things like refs in MD-friendly forms, eg class references can be done via [SomeClass](httpcore.SomeClass) (regular links).

What DID NOT change:

  • Scripts: scripts/docs serve and scripts/docs gh-deploy work the same way they did when using mkdocs. To deploy to GH Pages, we invoke ghp-import directly (MkDocs' gh-deploy is a wrapper around that) in the shell script. The resulting script remains pretty lightweight. :-)

@florimondmanca florimondmanca added the documentation Improvements or additions to documentation label Mar 21, 2021
* Add [source] links

Fix broken admonition in contributing.md

Use ::: for reST items

Document proxies

Update build and publish scripts

Simplify conf.py
@florimondmanca
Copy link
Member Author

florimondmanca commented Mar 21, 2021

For some reason I'm not able to get the [source] link to appear alongside class definitions — I only get it for methods. I checked the Sphinx bug tracker and saw nothing there. Also reproduces with Alabaster so probably not something with Furo. So I assume I'm doing something wrong? See eg requests.Session for what I'm after.

Edit: I found where that comes from.

We're forcing attr.__module__ = "httpcore" on all public attributes for prettier reprs, see #158.

That means viewcode treats all public items as if they were defined in httpcore/__init__.py, but they aren't, and as a result it's not able to find which module the attribute is actually defined in. See Sphinx's get_full_modname util, used by viewcode._get_full_modname, used by viewcode.doctree_read.

I was able to make the [source] and [docs] link appear and work correctly by enabling viewcode_follow_imported_members and defining an event handler that finds the original module manually. Pushing…

@florimondmanca florimondmanca requested a review from a team March 23, 2021 20:02
@florimondmanca
Copy link
Member Author

This is ready for review, I'm quite happy about this.

@tomchristie Any thoughts on this? I know the "consistency of tooling across Encode" argument will certainly pop in your mind. :-) Do you feel like playing with this based on the hints I gave, see how it all works in the day-to-day development process? There's not much really different from MkDocs as far as the writing experience goes, except that mkdocs.yml is replaced with conf.py + index.md for the table of contents / page list. I personally think moving to something like this would be hugely beneficial for HTTPX too, with a very low investment and maintenance effort for us given the maturity and stability of the Sphinx ecosystem.

docs/conf.py Outdated Show resolved Hide resolved
httpcore/__init__.py Outdated Show resolved Hide resolved
@JayH5
Copy link
Member

JayH5 commented Mar 23, 2021

I like this 😄. The output looks really good and you've written a really clear explanation of everything that's going on here.

To me the most controversial thing is that the markup language is kind of a weird mix. The main docs are markdown still, but then the Sphinx-specific stuff looks more like RST, and docstrings become a lot more like RST. You've explained why this is, I'm not sure much can be done about it, and I think it's probably worth the trade-offs.

@florimondmanca florimondmanca mentioned this pull request Mar 24, 2021
10 tasks
@tomchristie
Copy link
Member

Okay, so the docs do looks pretty great.

I've not downloaded and tried it out locally yet, that's something I want to do but don't have the time today.
Thoughts that come to mind...

  • Yes, as you say, consistency across Encode is a biggie. Or at least perhaps thinking about having both HTTPX + HTTPCore in line. There's valid concerns there about branding consistency - eg. If we were looking at aiming for a hub of packages under the https://github.com/python-http banner, then Sphinx likely works really well for fitting in with other projects. If we're aiming for a nicely tied in Encode suite of packages, then I think the MKdocs + material theming is generally easier to wrangle.
  • I'm super curious about the workflow for multi-language support in Sphinx. Working towards having docs in a wide range of languages ought to be a priority for us.
  • Also, how about multiple versioning?
  • Would we be better off hosted on readthedocs for any of these things?

I think we really do want the improved API Docs this gives us, so either we ought to be considering this, or else weighing up what work we'd need to put in to get MkDocs up to an equivalent in that area.

:::{eval-rst}
.. autoclass:: httpcore.PlainByteStream
:show-inheritance:
:noindex:
Copy link
Member

Choose a reason for hiding this comment

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

Curious - what is :noindex: here, and why on this class in particular?

Copy link
Member Author

Choose a reason for hiding this comment

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

We show this class two times, once for the sync API, once for the async API. I figured it's fine, except showing them twice would make Sphinx index them twice, which raised a warning hinting to add :noindex:.

The maximum number of connections to allow before closing keep-alive
connections.
http2:
Enable HTTP/2 support.
Copy link
Member

Choose a reason for hiding this comment

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

Have to say, I do prefer the style here after this change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Same!

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

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

@florimondmanca I think I'm probably happy to leave this one up to you.

It's a pretty difficult set of tradeoffs, but looking at the URLLib3 docs I really like how it can look with a little bit of nice branding etc thrown it.

The API documentation is clearer, for sure. That's a massive win.

Areas that'd be interesting to think about...

  • What would we need to do to get versioned docs?
  • What would we need to do to get internationalised docs, and how does that work with the Furo theme?
  • What (if anything) would we gain by hosting on ReadTheDocs?
  • I'm not necessarily against switching to RST for the documentation. Not that big of a deal perhaps.

@felix-hilden
Copy link
Member

I'll chime in for two points you raised: versioned docs would go hand in hand with hosting on RTD very easily. They allow for building based on Git tags. Commonly that means using two versions: latest (i.e. current master) and stable (latest tag). I think it's possible to activate builds for individual tags too, to for example freeze documentation for previous major versions.

@pradyunsg
Copy link

What would we need to do to get internationalised docs, and how does that work with the euro theme?

I imagine you meant Furo. Everything (except RTL) is purely handled by Sphinx's build pipeline, and doesn't need any additional support on the theme.

I don't know of any Sphinx theme that supports RTL out-of-the-box but I do plan to add this in Furo and set stuff up in sphinx-basic-ng, so that future themes that derive from it; won't have this problem. :)

https://jareddillard.com/blog/documentation-internationalization-using-sphinx-and-zanata is something I got linked to recently on this topic.

What would we need to do to get versioned docs?

ReadTheDocs. Host it there, and they'll give you the overlay for versioning your docs.

What (if anything) would we gain by hosting on ReadTheDocs?

Answered above. :)

@tomchristie
Copy link
Member

I imagine you meant Furo.

Oops - typo 🤪

@tomchristie
Copy link
Member

Short. Yup I'm okay with this, if and when @florimondmanca is.

The improvement in the API docs and the interlinking is motivation enough.

@florimondmanca
Copy link
Member Author

Happy with how this is turning out! I'll update the PR and ready to merge afterwards. :-)

@florimondmanca florimondmanca merged commit 57e43d8 into master Apr 9, 2021
@florimondmanca florimondmanca deleted the fm/docs-sphinx branch April 9, 2021 13:46
@florimondmanca
Copy link
Member Author

Merged, and just deployed docs via:

scripts/docs gh-deploy --push

See: b0fb17d

👍

@tomchristie
Copy link
Member

Took some time to speak with Eric Holscher of ReadTheDocs about getting us hosted there, in particular wrt. managing sponsorships etc.

Example of hosting it up on RTD, along with a sidebar ad demo'ing that we'd be able to manage our sponsorship placements...

https://httpcore.readthedocs.io/

@pradyunsg
Copy link

Ah, a custom publisher! Neat!

@tomchristie
Copy link
Member

Latest docs deployments have been failing.

https://github.com/encode/httpcore/actions/workflows/publish.yml

Not looked into it yet.

@tomchristie
Copy link
Member

Screenshot 2021-04-29 at 14 02 38

@pradyunsg
Copy link

error: failed to push some refs to 'https://github.com/encode/httpcore'
hint: Updates were rejected because the remote contains work that you do
hint: not have locally. This is usually caused by another repository pushing
hint: to the same ref. You may want to first integrate the remote changes
hint: (e.g., 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

Seems to be a git-related issue.

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

Successfully merging this pull request may close these issues.

None yet

5 participants