[PATCH 0/1] sphinx/qapidoc: pylint cleanups

[PATCH 0/1] sphinx/qapidoc: pylint cleanups

[Markus Armbruster]

There are a few more reports left.  Before I try to address them, I'd
like to know:

1. Do we still need to support Sphinx older than 1.7?

2. What should intersperse() do when the first argument is empty?

Markus Armbruster (1):
  sphinx/qapidoc: Tidy up pylint warning raise-missing-from

 docs/sphinx/qapidoc.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.41.0

[PATCH 1/1] sphinx/qapidoc: Tidy up pylint warning raise-missing-from

[Markus Armbruster]

Pylint advises:

    docs/sphinx/qapidoc.py:518:12: W0707: Consider explicitly re-raising using 'raise ExtensionError(str(err)) from err' (raise-missing-from)

From its manual:

    Python's exception chaining shows the traceback of the current
    exception, but also of the original exception.  When you raise a
    new exception after another exception was caught it's likely that
    the second exception is a friendly re-wrapping of the first
    exception.  In such cases `raise from` provides a better link
    between the two tracebacks in the final error.

Makes sense, so do it.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 docs/sphinx/qapidoc.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
index 8f3b9997a1..658c288f8f 100644
--- a/docs/sphinx/qapidoc.py
+++ b/docs/sphinx/qapidoc.py
@@ -515,7 +515,7 @@ def run(self):
         except QAPIError as err:
             # Launder QAPI parse errors into Sphinx extension errors
             # so they are displayed nicely to the user
-            raise ExtensionError(str(err))
+            raise ExtensionError(str(err)) from err
 
     def do_parse(self, rstlist, node):
         """Parse rST source lines and add them to the specified node
-- 
2.41.0

Re: [PATCH 1/1] sphinx/qapidoc: Tidy up pylint warning raise-missing-from

[John Snow]

On Wed, Oct 25, 2023 at 6:10 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Pylint advises:
>
>     docs/sphinx/qapidoc.py:518:12: W0707: Consider explicitly re-raising using 'raise ExtensionError(str(err)) from err' (raise-missing-from)
>
> From its manual:
>
>     Python's exception chaining shows the traceback of the current
>     exception, but also of the original exception.  When you raise a
>     new exception after another exception was caught it's likely that
>     the second exception is a friendly re-wrapping of the first
>     exception.  In such cases `raise from` provides a better link
>     between the two tracebacks in the final error.
>
> Makes sense, so do it.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

In this case it probably doesn't make a difference because Sphinx has
its own formatting for displaying the errors, but it's good hygiene.

Reviewed-by: John Snow <jsnow@redhat.com>

> ---
>  docs/sphinx/qapidoc.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/docs/sphinx/qapidoc.py b/docs/sphinx/qapidoc.py
> index 8f3b9997a1..658c288f8f 100644
> --- a/docs/sphinx/qapidoc.py
> +++ b/docs/sphinx/qapidoc.py
> @@ -515,7 +515,7 @@ def run(self):
>          except QAPIError as err:
>              # Launder QAPI parse errors into Sphinx extension errors
>              # so they are displayed nicely to the user
> -            raise ExtensionError(str(err))
> +            raise ExtensionError(str(err)) from err
>
>      def do_parse(self, rstlist, node):
>          """Parse rST source lines and add them to the specified node
> --
> 2.41.0
>
>

Re: [PATCH 0/1] sphinx/qapidoc: pylint cleanups

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> There are a few more reports left.  Before I try to address them, I'd
> like to know:
>
> 1. Do we still need to support Sphinx older than 1.7?
>
> 2. What should intersperse() do when the first argument is empty?

Patch queued, questions remain open.

[...]

Re: [PATCH 1/1] sphinx/qapidoc: Tidy up pylint warning raise-missing-from

[Peter Maydell]

On Fri, 3 Nov 2023 at 03:08, John Snow <jsnow@redhat.com> wrote:
>
> On Wed, Oct 25, 2023 at 6:10 AM Markus Armbruster <armbru@redhat.com> wrote:
> >
> > Pylint advises:
> >
> >     docs/sphinx/qapidoc.py:518:12: W0707: Consider explicitly re-raising using 'raise ExtensionError(str(err)) from err' (raise-missing-from)
> >
> > From its manual:
> >
> >     Python's exception chaining shows the traceback of the current
> >     exception, but also of the original exception.  When you raise a
> >     new exception after another exception was caught it's likely that
> >     the second exception is a friendly re-wrapping of the first
> >     exception.  In such cases `raise from` provides a better link
> >     between the two tracebacks in the final error.
> >
> > Makes sense, so do it.
> >
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> In this case it probably doesn't make a difference because Sphinx has
> its own formatting for displaying the errors, but it's good hygiene.
>
> Reviewed-by: John Snow <jsnow@redhat.com>

Has somebody checked that the error Sphinx shows to the user
is still the friendly one? The only reason to raise
this error is so that Sphinx will catch it and display
the friendly string, so anything about tracebacks is a red
herring -- if the traceback is shown to the user then we got
something wrong.

thanks
-- PMM

Re: [PATCH 1/1] sphinx/qapidoc: Tidy up pylint warning raise-missing-from

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Fri, 3 Nov 2023 at 03:08, John Snow <jsnow@redhat.com> wrote:
>>
>> On Wed, Oct 25, 2023 at 6:10 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> > Pylint advises:
>> >
>> >     docs/sphinx/qapidoc.py:518:12: W0707: Consider explicitly re-raising using 'raise ExtensionError(str(err)) from err' (raise-missing-from)
>> >
>> > From its manual:
>> >
>> >     Python's exception chaining shows the traceback of the current
>> >     exception, but also of the original exception.  When you raise a
>> >     new exception after another exception was caught it's likely that
>> >     the second exception is a friendly re-wrapping of the first
>> >     exception.  In such cases `raise from` provides a better link
>> >     between the two tracebacks in the final error.
>> >
>> > Makes sense, so do it.
>> >
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>
>> In this case it probably doesn't make a difference because Sphinx has
>> its own formatting for displaying the errors, but it's good hygiene.
>>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>
> Has somebody checked that the error Sphinx shows to the user
> is still the friendly one? The only reason to raise
> this error is so that Sphinx will catch it and display
> the friendly string, so anything about tracebacks is a red
> herring -- if the traceback is shown to the user then we got
> something wrong.

The exception type doesn't change, only the backtrace stored within the
exception.  Can't see how its catching could be affected.

To be sure, stick a '} at the beginning of qapi-schema.json and run
sphinx-build.  Result:

    Extension error:
    /work/armbru/qemu/docs/../qapi/qapi-schema.json:1:1: expected '{', '[', string, or boolean

Satisfied?

Re: [PATCH 1/1] sphinx/qapidoc: Tidy up pylint warning raise-missing-from

[Peter Maydell]

On Fri, 3 Nov 2023 at 16:02, Markus Armbruster <armbru@redhat.com> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > On Fri, 3 Nov 2023 at 03:08, John Snow <jsnow@redhat.com> wrote:
> >>
> >> On Wed, Oct 25, 2023 at 6:10 AM Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> > Pylint advises:
> >> >
> >> >     docs/sphinx/qapidoc.py:518:12: W0707: Consider explicitly re-raising using 'raise ExtensionError(str(err)) from err' (raise-missing-from)
> >> >
> >> > From its manual:
> >> >
> >> >     Python's exception chaining shows the traceback of the current
> >> >     exception, but also of the original exception.  When you raise a
> >> >     new exception after another exception was caught it's likely that
> >> >     the second exception is a friendly re-wrapping of the first
> >> >     exception.  In such cases `raise from` provides a better link
> >> >     between the two tracebacks in the final error.
> >> >
> >> > Makes sense, so do it.
> >> >
> >> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >>
> >> In this case it probably doesn't make a difference because Sphinx has
> >> its own formatting for displaying the errors, but it's good hygiene.
> >>
> >> Reviewed-by: John Snow <jsnow@redhat.com>
> >
> > Has somebody checked that the error Sphinx shows to the user
> > is still the friendly one? The only reason to raise
> > this error is so that Sphinx will catch it and display
> > the friendly string, so anything about tracebacks is a red
> > herring -- if the traceback is shown to the user then we got
> > something wrong.
>
> The exception type doesn't change, only the backtrace stored within the
> exception.  Can't see how its catching could be affected.
>
> To be sure, stick a '} at the beginning of qapi-schema.json and run
> sphinx-build.  Result:
>
>     Extension error:
>     /work/armbru/qemu/docs/../qapi/qapi-schema.json:1:1: expected '{', '[', string, or boolean
>
> Satisfied?

Yep, looks good, thanks. (That is, this change is purely
to shut up pylint, and it doesn't have any bad side effects.)

-- PMM