On Fri, Jan 26, 2024 at 09:30:29AM +0100, Peter Krempa wrote:
> On Thu, Jan 25, 2024 at 09:06:54 -0800, Andrea Bolognani wrote:
> > I've never seen the 'raise Foo() from None' pattern though. Can you
> > explain why it's needed here?
>
> Python would re-raise the original exception along with the new one
> without that.
I guess it doesn't hurt to have it there, but it doesn't seem very
useful either:
$ git diff
diff --git a/scripts/qemu-replies-tool.py b/scripts/qemu-replies-tool.py
index 9ab1c30ee2..bde6b2dad5 100755
--- a/scripts/qemu-replies-tool.py
+++ b/scripts/qemu-replies-tool.py
@@ -46,7 +46,7 @@ def qemu_replies_load(filename):
jsonstr = ''
except json.decoder.JSONDecodeError as je:
- raise qrtException("JSON error:\n'%s'\nwhile processing
snippet:\n'%s'" % (je, jsonstr)) from None
+ raise qrtException("JSON error:\n'%s'\nwhile processing
snippet:\n'%s'" % (je, jsonstr))
if command is not None or jsonstr != '':
if command is not None:
diff --git a/tests/qemucapabilitiesdata/caps_8.2.0_x86_64.replies
b/tests/qemucapabilitiesdata/caps_8.2.0_x86_64.replies
index cc2190dfd3..cf1425e0bb 100644
--- a/tests/qemucapabilitiesdata/caps_8.2.0_x86_64.replies
+++ b/tests/qemucapabilitiesdata/caps_8.2.0_x86_64.replies
@@ -1,5 +1,5 @@
{
- "execute": "qmp_capabilities",
+ "execute": } "qmp_capabilities",
"id": "libvirt-1"
}
$ ./scripts/qemu-replies-tool.py
tests/qemucapabilitiesdata/caps_8.2.0_x86_64.replies
'tests/qemucapabilitiesdata/caps_8.2.0_x86_64.replies' ... FAIL
JSON error:
'Expecting value: line 2 column 14 (char 15)'
while processing snippet:
'{
"execute": } "qmp_capabilities",
"id": "libvirt-1"
}
'
The behavior without it is just the same AFAICT.
> > The way these examples are all jumbled together makes it IMO a lot
> > harder to understand what each is supposed to do, or even where one
> > ends and the next one starts. I suggest you do something like this
> > instead:
> >
> > def modify_replies(conv):
> > return # no changes
> >
> > def _modify_replies_example1(conv):
> > # ...
> >
> > def _modify_replies_example2(conv):
> > # ...
>
> The above is really just one example though:
> - it shows version lookup
> - lookup of a command in the replies list
> - insertion of a new command into the list:
> - if the version doesn't match
> - add an error
> - if the version does match
> - add the real data
>
> The above is a common pattern if you want to add probe for a new device.
>
> It just shows two distinct (but with a reason) versions how to get the
> JSON data. The first one is constructing it manually in python, which is
> reasonable for the command or error. The second one is loading it from a
> JSON string you'd copy&paste from qemu.
Okay, I clearly completely misunderstood what the code was trying to
do, arguably proving that it's not as obvious to someone else as it
certainly is to you :)
It's fine to keep the function as is, but please add some comments
explaining what it does and why. Some variation of the above will do
nicely.
> > Honestly I would prefer it if the way to make the script process
> > multiple files was just to list them all on the command line, but I
>
> I think this is simple thing to do with the argument parser in python
Oh, absolutely! It's just a shame that we can't do things that way
because of meson.
--
Andrea Bolognani / Red Hat / Virtualization
_______________________________________________
Devel mailing list -- [email protected]
To unsubscribe send an email to [email protected]
