| Message ID | 20231025092159.1782638-2-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | sphinx/qapidoc: pylint cleanups | expand |
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 > >
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
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?
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
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
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(-)