Closed jakkdl closed 1 month ago
For generating rules pages, I've looked at https://www.sphinx-doc.org/en/master/man/sphinx-autogen.html and https://www.sphinx-doc.org/en/master/usage/extensions/autosummary.html#directive-autosummary and considered making visitors classes with the extra documentation in their docstrings. But I think that's going to cause issues - primarily in having large limitations on the formatting of the resulting rules subpages. The page would essentially be the documentation of a single class, with code examples etc etc in the docstring.
The easiest would be manually writing the subpages, and that's probably what I'll start with for the sake of exploring layout etc etc, but this will be error-prone/cumbersome/hard to maintain. I think in the end I'll have to write a custom generating script, that reads the data from the visitor classes (means it has to be able to handle multiple codes per visitor) or an entirely separate system.
I'm thinking something like:
Visitor91x.documentation_examples('ASYNC910')
documentation_blurb
: for use in the overview table in rules.rst (equivalent to ruff "Message" column)documentation_trigger
: describes what will trigger the rule (equivalent to what it does).documentation_info
: longer explanation (why is this bad)documentation_examples
: examples that trigger the error, and how to fix them. (example).
eval_files
, but that'd hit problems with library-specific errors and all the special MagicMarkers.tests/doc_examples/
(+autofix?) that can both be run as tests, get formatted, etc - and then get sourced straight into the documentation. This does smell like overengineering, but we do have >30 rules...rst
files will be mostly straightforward.
rules.rst
we can use the csv-table directive which supports reading the content from a local .csv
file. Dumping the blurbs+error codes+human-readable into a csv is straigthforward. (and including a link to the individual rules pages should be as well)docs/Makefile
. Either as a target that tries to be fancy, or just add it as a commmand to the catch-all target.thoughts?
Let's start by just writing all the subpages by hand; I think that the complexity of generating stuff probably isn't worth it at the moment (given some classes implement multiple rules, etc).
The main downside is having to write, and keep in sync,
But yeah the smart thing is probably to start doing it manually, although I kind of dread doing so 30 times over. But I guess we don't have to smash out detailed rules pages for all rules pages at the same time, can start with a couple and leave the rest with long blurbs.
Separate glossary page sounds great to me, as does matching Ruff's structure for the docs of each rule. That might be a bit much to put into one PR though; I'd be keen to merge this in small pieces if we can - e.g. rearrange docs in this one, then follow-ups to add the _info and _examples sections, and then consider tests or automation.
Sounds good. Will see if I clean up this one and write follow-up PRs, and/or split out smaller PRs from it.
One thing we should do ASAP is prominently link from the github readme and the pypi page directly to the rules page in our docs.
We additionally made a release with #244 but none with #245 or later, so PyPI still runs with the old README page.
Ended up going the clean-up-this-one-and-then-do-followup-PRs:
README.md
The PyPI readme is the same@augustelalande @charliermarsh in this PR I've started adding human-friendly rule names (see #236), in large part inspired by you guys. So I thought I'd compare with the names you'd assigned in https://github.com/astral-sh/ruff/pull/10416
(We're also planning on mostly copying your setup of having subpages for each rule with a longer description, examples, etc; when that's done you can probably straight-up copy-paste those sections from us to your docs.)
General observations:
InAsyncFunction
is mostly redundant within the scope of flake8-async. But maybe there's enough upsides of matching the human-friendly names between ruff&us that it outweighs having to have a longer name.(due to where I've copy-pasted from their rule names are formatted in camel case, but treat them as they're words-separated-by-dashes)
100 TrioTimeoutWithoutAwait - cancel-scope-no-checkpoint
our name is much better here, they should probably conform.
105 TrioSyncCall - missing-await
I think missing-await
is clearly better since there's plenty sync trio functions.
109 TrioAsyncFunctionWithTimeout - async-function-with-timeout
110 TrioUnneededSleep - busy-wait
"unneeded-sleep" sounds like the sleep isn't necessary and can be removed, but that's far from the case.
115 TrioZeroSleepCall - sleep-zero
lol
210 BlockingHttpCallInAsyncFunction - blocking-http-call
220 CreateSubprocessInAsyncFunction - blocking-process-call-1
221 RunProcessInAsyncFunction - blocking-process-call-2
222 WaitForProcessInAsyncFunction - blocking-process-call-3
I very much ran out of energy trying to come up with better names, so might go with the ruff ones unless we can come up with a more descriptive difference between the rules.
230 BlockingOpenCallInAsyncFunction - blocking-io-call
Idk about io
vs Open
.
251 BlockingSleepInAsyncFunction - blocking-sleep
great minds think alike
Personally I'm fine with your proposed names, so I'll update my PR with whatever you finalize on here.
*except blocking-process-call-1,2,3 😉
The checkpoint
glossary entry is... absolutely awful. And there's no direct documentation on it for anyio, and I'm not finding much on how works in asyncio. @Zac-HD plx intervene (or I can mark it as WIP/TODO and ship it for now). I also asked about it briefly on gitter.
I added async
to some names that were somewhat ambiguous (e.g. "zero-sleep" might imply that it would trigger on time.sleep(0)
). This doesn't matter when it comes to # noqa
, but makes seeing the names in configs better and might be appreciated by ruff. Also fixed the 2xx names
busy-wait
-> async-busy-wait
sleep-zero
-> async-zero-sleep
blocking-call
-> blocking-configured-call
blocking-process-call-1
-> blocking-create-subprocess
blocking-process-call-2
-> blocking-run-process
blocking-process-call-3
-> blocking-process-wait
blocking-io-call
-> blocking-open-call
blocking-io-call-wrap
-> blocking-fdopen-call
Other than the checkpoint glossary entry I think I'm happy with this PR now.
rewrote the checkpoint glossary entry after discussions in gitter, so this is ready for final review & merge
Various rules doc improvements:
WIP tasks
yield-in-nursery-or-cancelscope
oryield-in-taskgroup-or-cancelscope
oryield-in-nursery-or-taskgroup-or-cancelscope
. (and should it bescope
, orcancelscope
, orcancelscope-or-timeout
, or justtimeout
, etc.). Can be fixed with glossary etc when reading docs, but we're also considering people stumbling upon these names innoqa
statements or in configs.hoverxref
additional tasks I have not started on:
I might split out some stuff (e.g. intersphinx) into separate PRs.