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