Docs and command help improvements #299
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Easier and more conventional than remembering you can use, e.g., SPHINXOPTS=--host=X.
We use Sphinx's autodoc to generate documentation from the source code, so it's useful to have the docs auto rebuild when writing docstrings.
For good measure, even if in practice there's never a "livehtml" file/directory that exists to muck things up.
…uto-rebuilding Sometimes auto-rebuilding is too much.
It'll be useful soon elsewhere in development tooling and is generic to all chains of nested commands in argparse. Move it before I improve it.
We can get it from the parser's "prog" property. Since it's now optional, reverse the argument order of walk_commands().
This avoids things like testing the --help output of both `nextstrain remote list` and `nextstrain remote ls`. The primary name is visited first, so it's the aliases we'll skip. Note that this only applies to simple aliases (i.e. using the "aliases" feature of argparse), not more complex aliases like our `nextstrain deploy`.
While the original use case in tests/help.py didn't need the parser, other upcoming use cases will. It also makes the function more general. The original use case can simply discard the parser.
Covers the extended parts of help output we were not previously testing.
tsibley
force-pushed
the
trs/docs-and-help
branch
2 times, most recently
from
4d566c3 to
9c6332a
Compare
This ditches sphinx-argparse and the "argparse" directive in favor of our own static generation of rST from the argument parsers. The main motivation here is to emit the Sphinx "program" and "option" directives in these pages so we can use the Sphinx "option" role (e.g. :option:`--foo`) to cross-reference command options. Cross-referencing like this is very useful because it means readers don't have to chase down the docs for an unfamiliar option themselves. Secondary motivations include having more control over the formatting of the pages and making more apparent the overall structure of the pages (since we're both authoring the rST source and can see the generated output, unlike with the "argparse" directive). As an example of where more formatting control is useful: now we can put the usage example at the very top, above the command description, like it is in --help output. The "argparse" directive put the usage below the description, but this made descriptions confusing since they often refer to placeholders in the usage. The statically generated files are checked into version control for visibility and because they should be the exact same for any maintainer. To ensure the copies in version control stay up-to-date, the tests assert that generated docs are up-to-date. To ensure that the built docs stay up-to-date even if version control diverges, the docs build process automatically regenerates the files if needed. Note that while generate-command-doc accesses private argparse internals, this is precisely what sphinx-argparse does as well so we're not any less insulated from breakage. The private internals we use are limited and, given historical development of argparse, seem unlikely to change routinely.
…help strings Use the former for examples of command invocations and the former for generic literal text snippets. In the text terminal context, this leaves the generic literals unadorned (they're already readable as-is) and the command invocations surrounded by backticks (so they stand out as such from surrounding text). In the web context, both forms are distinguished from the surrounding text the same way.
This bit of rST:
See :command-reference:`nextstrain remote` for more details.
now renders in the text terminal context as:
See `nextstrain build --help` for more details.
and in the web context renders "nextstrain build" as a link to the
command's doc page.
There are fancier/better approaches—such as creating an extension and
using Sphinx.add_object_type()¹—but the approach here seemed like the
simplest way to address the different capabilities of the terminal vs.
web. We may want to switch to a different approach in the future,
however.
¹ <https://www.sphinx-doc.org/en/master/extdev/appapi.html#sphinx.application.Sphinx.add_object_type>
Hardcoding isn't always suitable, e.g. at narrower terminal sizes or when rendering small chunks of rST for use in a larger block of formatted text. There were several ways to thread thru the configured width in the rst.sphinx code, all inherited from upstream, so I tried to take the way most like other settings.
The help text for options is now wrapped sensibly—previously only the command description was—and the max overall width for all parts of --help output will now abide by a readable length.
…for improved formatting and cross-linking on the web. Enabling this is that option descriptions in --help output are now rendered as rST → text, bringing them up to parity with command descriptions.
This gives those commands—`nextstrain`, `nextstrain debugger`, and `nextstrain init-shell`—a documentation page which can be directly visited but is unlinked by the rest of the docs.
The Sphinx "doc" role requires a full document name, i.e. being explicit about "index" pages. This makes sense as it's using filesystem semantics. Our rendering of rST to text for command-line help, however, turns "doc" roles into URLs and assumed URL semantics, i.e. where "/index.html" can be left implicit. This led to a semantic mismatch, which I noticed due to a "unknown reference" warning during the Sphinx build now that we include a (unlisted) doc page for `nextstrain`. Resolve the mismatch by using the standard Sphinx filesystem semantics in all our usage of the "doc" role and extend our rST to text conversion to explicitly handle the filesystem → URL semantic shift.
…ate fails Very helpful to see what's different. I built the diffing into generate-command-doc as a new --diff option since it will probably be handy in local dev too in combination with --check.
tsibley
force-pushed
the
trs/docs-and-help
branch
from
dd92478 to
4aaf67b
Compare
argparse in Python ≥3.10 uses "options" while earlier versions use "optional arguments". Our default during doc generation was already the slightly-nicer "options", so normalize to that regardless of Python version.
…default
This is an already established pattern we use with other commands where
we want nicer usage specs than what argparse provides. With this commit
we preserve the:
[<runtime> [<runtime> ...]]
notation on all versions of argparse, regardless of Python version.
That's the form produced by argparse on Python <3.10, but later versions
would shorten it to:
[<runtime> ...]
which is less descriptive in my eyes.
Like our other existing examples of custom usage, we take the
opportunity to break out -h from normal invocations and expand it to
--help for clarity.
This requirement was a well-intentioned attempt to try to avoid introducing backwards compatibility issues that can arise if, e.g., developing on 3.10 and not realizing something won't work on 3.6. But it turns out to make testing locally on different versions of Python less convenient since devel/setup-venv can't be used. Rely on CI tests to catch backwards/forwards compatibility issues instead.
To quickly switch between Python versions in local dev, I will often have copies of the .venv directories (i.e. the ones produced by devel/setup-venv) as .venv-3.10, .venv-3.8, etc. Flake8 takes ~forever to process those, and we don't want it to anyway. Ignore them.
Our development tooling requires a POSIX platform.
Catches warnings earlier during local builds, instead of only during CI. sphinx-autobuild doesn't have a released version which supports --keep-going¹, so be a little looser there. These options aren't the default for SPHINXOPTS so that that variable may still be used for passing additional options without unintentionally (and unnecessarily) overriding the strictures. ¹ <sphinx-doc/sphinx-autobuild#133>
There was a problem hiding this comment.
Had this on my to-do list thinking it was the runtime docs updates, which it's not... but for what it's worth I skimmed through each commit and these changes all seem fine to me. It'd still be great to have argparse rST parsing in augur, but I realise that's a whole nother push.
This makes them linkable/referencable (e.g. with the "envvar" role) and indexed in the web docs and resolves warnings in the Sphinx build about missing reference targets. Since "envvar" is a Sphinx directive, not a core docutils/rST directive, I've implemented a basic version of it for our rst_to_text() function used for --help output.
|
Ha, yeah. It's on the (twisty) path to those… things I decided to fix/improve as I noticed them as part of making those runtime docs. If we want to have a big focused doc push on Augur, we could definitely port/extract the argparse/rST stuff... |
|
There were a series of small issues revealed by CI that I hadn't anticipated, so good for CI I guess! I believe with the last push just now that CI should pass and this should be good to go (well, after changelogging). |
|
CI passed for the commit before the changelog commit. Doc preview looks good. I'm going to merge given @jameshadfield's 👀 on most of this already but welcome additional review! I don't expect to cut a new release until after some other doc updates/additions are made for runtimes. |
|
Nice overall improvements! I really like that the addition of direct links to command line arguments. My only suggestion is add a blurb that explicitly states that command line doc changes need to be manually updated via |
|
Ah, that's fair. Done in e2420d4! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A bunch of cumulative improvements to how we handle docs and command help. See commits for details, naturally.
Testing
--helpoutput for various commands looks good