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

Rules documentation improvements #248

Merged
merged 15 commits into from
May 22, 2024
Merged

Conversation

jakkdl
Copy link
Member

@jakkdl jakkdl commented May 14, 2024

Various rules doc improvements:

WIP tasks

  • table-style display of rules a la ruff, first step of fixing Generate proper documentation #214. Contains a shorter message blurb.
    * ruff copies over error message instead of having a custom blurb: https://docs.astral.sh/ruff/rules/ - that might be good to do to reduce the number of places that documentation is written. Will also need to define the human-friendly rule name in the code anyway, so this whole table should probably be generated one way or another....
  • human-friendly rule names, fixing Come up with human-friendly names for all error codes, and support those in --disable etc #236. I did not want to do big rules doc makeover without also incorporating this.
    • would love opinions on these. And unclear which library to refer to in the names. E.g. should it be yield-in-nursery-or-cancelscope or yield-in-taskgroup-or-cancelscope or yield-in-nursery-or-taskgroup-or-cancelscope. (and should it be scope, or cancelscope, or cancelscope-or-timeout, or just timeout, etc.). Can be fixed with glossary etc when reading docs, but we're also considering people stumbling upon these names in noqa statements or in configs.
  • Glossary: When writing human-friendly rule names and shorter descriptions I really wanted to be able to refer to a glossary that defined terms and linked to further information. This should go into a different page, and would be great with hoverxref

additional tasks I have not started on:

I might split out some stuff (e.g. intersphinx) into separate PRs.

@jakkdl
Copy link
Member Author

jakkdl commented May 16, 2024

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:

  • Each visitor defines a set of methods that return (rst) strings, one method per section in the rules page, and each method takes the error code as an argument.
    • e.g. Visitor91x.documentation_examples('ASYNC910')
    • Might as well copy the sections from ruff + blurb:
      • 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).
        • I'm somewhat tempted to source these directly from the eval_files, but that'd hit problems with library-specific errors and all the special MagicMarkers.
        • But writing sections that are going to be mostly code in strings sound bleh (formatting, syntax errors, etc), so maaybe add a new directory 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...
  • python script that generates rst files will be mostly straightforward.
    • We already have ways of finding all visitor classes, and iterating over all their error codes.
    • Generating the individual rst pages from the methods named above is trivial.
    • For generating the table in 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)
  • The easiest way of hooking in the script is probably to add it to docs/Makefile. Either as a target that tries to be fancy, or just add it as a commmand to the catch-all target.

thoughts?

@jakkdl jakkdl mentioned this pull request May 16, 2024
Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

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).

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.

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.

docs/rules.rst Outdated Show resolved Hide resolved
docs/rules.rst Outdated Show resolved Hide resolved
docs/rules.rst Outdated Show resolved Hide resolved
docs/rules.rst Outdated Show resolved Hide resolved
@jakkdl
Copy link
Member Author

jakkdl commented May 17, 2024

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,

  1. error message in visitor
  2. blurb in docs/rules.rst
  3. explanation in docs/rules/xxx.rst
  4. and of course the actual implementation & tests.

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.

@jakkdl jakkdl marked this pull request as ready for review May 17, 2024 14:53
@jakkdl
Copy link
Member Author

jakkdl commented May 17, 2024

Ended up going the clean-up-this-one-and-then-do-followup-PRs:

  • wrote human-friendly rulenames for all rules. They're not usable anywhere, but that shouldn't come in too long so we can probably just leave that for the moment.
    • TODO: check the names against ruff names; check for collisions, compare with the names they've already given for ASYNCxxx/TRIOxxx
  • I temporarily abandoned blurb-should-be-shorter, copied back the extra context that rules used to have, and made minimal changes to blurbs for the other rules when converting to table-style. Leaving this for when rules actually get broken out into separate subpages.
  • added the requested prominent link to README.md The PyPI readme is the same
  • the new glossary entries I added needs some moar writing

@jakkdl
Copy link
Member Author

jakkdl commented May 17, 2024

@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 astral-sh/ruff#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:

  1. Several of ruff's rules are still named trio-xxx, which almost surely should change since there's very few rules that actually are trio-specific. I think in all cases these are leftovers from when flake8-async/flake8-trio were separate.
  2. 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

@augustelalande
Copy link

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 😉

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

non-blocking comments, looks great overall 🙏

docs/glossary.rst Outdated Show resolved Hide resolved
docs/rules.rst Outdated Show resolved Hide resolved
@jakkdl
Copy link
Member Author

jakkdl commented May 20, 2024

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

  • 110: busy-wait -> async-busy-wait
  • 115: sleep-zero -> async-zero-sleep
  • 200: blocking-call -> blocking-configured-call
  • 220: blocking-process-call-1 -> blocking-create-subprocess
  • 221: blocking-process-call-2 -> blocking-run-process
  • 222: blocking-process-call-3 -> blocking-process-wait
  • 230: blocking-io-call -> blocking-open-call
  • 231: blocking-io-call-wrap -> blocking-fdopen-call

Other than the checkpoint glossary entry I think I'm happy with this PR now.

@jakkdl
Copy link
Member Author

jakkdl commented May 22, 2024

rewrote the checkpoint glossary entry after discussions in gitter, so this is ready for final review & merge

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

Pushed some formatting tweaks and slight rewording for the checkpoint entry - let's go!

@Zac-HD Zac-HD enabled auto-merge (squash) May 22, 2024 18:20
@Zac-HD Zac-HD merged commit b9b0869 into python-trio:main May 22, 2024
10 checks passed
@jakkdl jakkdl deleted the doc_improvements branch August 31, 2024 13:25
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.

3 participants