Re: [PATCH 03/20] docs/qapidoc: delint a tiny portion of the module

[John Snow <jsnow@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6047 bytes --]

On Wed, May 15, 2024, 1:27 PM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > On Wed, May 15, 2024 at 5:17 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > In the coming patches, it's helpful to have a linting baseline.
> However,
> >> > there's no need to shuffle around the deck chairs too much, because
> most
> >> > of this code will be removed once the new qapidoc generator (the
> >> > "transmogrifier") is in place.
> >> >
> >> > To ease my pain: just turn off the black auto-formatter for most, but
> >> > not all, of qapidoc.py. This will help ensure that *new* code follows
> a
> >> > coding standard without bothering too much with cleaning up the
> existing
> >> > code.
> >> >
> >> > For manual checking for now, try "black --check qapidoc.py" from the
> >> > docs/sphinx directory. "pip install black" (without root permissions)
> if
> >> > you do not have it installed otherwise.
> >> >
> >> > Signed-off-by: John Snow <jsnow@redhat.com>
> >> > ---
> >> >  docs/sphinx/qapidoc.py | 16 +++++++++-------
> >> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >> >
> >> > diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> >> > index f270b494f01..1655682d4c7 100644
> >> > --- a/docs/sphinx/qapidoc.py
> >> > +++ b/docs/sphinx/qapidoc.py
> >> > @@ -28,28 +28,30 @@
> >> >  import re
> >> >
> >> >  from docutils import nodes
> >> > +from docutils.parsers.rst import Directive, directives
> >> >  from docutils.statemachine import ViewList
> >> > -from docutils.parsers.rst import directives, Directive
> >> > -from sphinx.errors import ExtensionError
> >> > -from sphinx.util.nodes import nested_parse_with_titles
> >> > -import sphinx
> >> > -from qapi.gen import QAPISchemaVisitor
> >> >  from qapi.error import QAPIError, QAPISemError
> >> > +from qapi.gen import QAPISchemaVisitor
> >> >  from qapi.schema import QAPISchema
> >> >
> >> > +import sphinx
> >> > +from sphinx.errors import ExtensionError
> >> > +from sphinx.util.nodes import nested_parse_with_titles
> >> > +
> >>
> >> Exchanges old pylint gripe
> >>
> >>     docs/sphinx/qapidoc.py:45:4: C0412: Imports from package sphinx are
> >> not grouped (ungrouped-imports)
> >>
> >> for new gripes
> >>
> >>     docs/sphinx/qapidoc.py:37:0: C0411: third party import "import
> sphinx"
> >> should be placed before "from qapi.error import QAPIError, QAPISemError"
> >> (wrong-import-order)
> >>     docs/sphinx/qapidoc.py:38:0: C0411: third party import "from
> >> sphinx.errors import ExtensionError" should be placed before "from
> >> qapi.error import QAPIError, QAPISemError" (wrong-import-order)
> >>     docs/sphinx/qapidoc.py:39:0: C0411: third party import "from
> >> sphinx.util.nodes import nested_parse_with_titles" should be placed
> before
> >> "from qapi.error import QAPIError, QAPISemError" (wrong-import-order)
> >>
> >> Easy enough to fix.
> >>
> >
> > I believe these errors are caused by the fact that the tools are confused
> > about the "sphinx" namespace - some interpret them as being the local
> > "module", the docs/sphinx/ directory, and others believe them to be the
> > third party external package.
> >
> > I have not been using pylint on docs/sphinx/ files because of the
> > difficulty of managing imports - this environment is generally beyond the
> > reaches of my python borgcube and at present I don't have plans to
> > integrate it.
> >
> > At the moment, I am using black, isort and flake8 for qapidoc.py and
> > they're happy with it. I am not using mypy because I never did the typing
> > boogaloo with qapidoc.py and I won't be bothering - except for any new
> code
> > I write, which *will* bother. By the end of the new transmogrifier,
> > qapidoc.py *will* strictly typecheck.
> >
> > pylint may prove to be an issue with the imports, though. isort also
> seems
> > to misunderstand "sphinx, the stuff in this folder" and "sphinx, the
> stuff
> > in a third party package" and so I'm afraid I don't have any good ability
> > to get pylint to play along, here.
> >
> > Pleading for "Sorry, this sucks and I can't figure out how to solve it
> > quickly". Maybe a future project, apologies.
>
> Is this pain we inflict on ourselves by naming the directory "sphinx"?
>

More or less, yeah. If you check the file from a CWD where there is no
"sphinx" directory, it behaves more normally.

Just not worth renaming it and futzing about for now. However, I did get an
invocation that lets me get a clean pylint run by abusing PYTHONPATH again,
so I have at least one standard baseline we can count on. I updated the
do-not-merge patch to include the special magick incantations.

Maybe in the future I'll make a qemu.plugins submodule instead, but that's
for quite a bit later.


> >> >
> >> >  # Sphinx up to 1.6 uses AutodocReporter; 1.7 and later
> >> >  # use switch_source_input. Check borrowed from kerneldoc.py.
> >> > -Use_SSI = sphinx.__version__[:3] >= '1.7'
> >> > +Use_SSI = sphinx.__version__[:3] >= "1.7"
> >> >  if Use_SSI:
> >> >      from sphinx.util.docutils import switch_source_input
> >> >  else:
> >> >      from sphinx.ext.autodoc import AutodocReporter
> >> >
> >> >
> >> > -__version__ = '1.0'
> >> > +__version__ = "1.0"
> >> >
> >> >
> >> > +# fmt: off
> >>
> >> I figure this tells black to keep quiet for the remainder of the file.
> >> Worth a comment, I think.
> >>
> >> >  # Function borrowed from pydash, which is under the MIT license
> >> >  def intersperse(iterable, separator):
> >> >      """Yield the members of *iterable* interspersed with
> *separator*."""
> >>
> >> With my comments addressed
> >> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> >>
> >
> > ^ Dropping this unless you're okay with the weird import orders owing to
> > the strange import paradigm in the sphinx folder.r
>
> Feel free to keep it.
>
>

[-- Attachment #2: Type: text/html, Size: 8461 bytes --]