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

[Markus Armbruster]

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"?

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