Re: [PATCH v4 25/35] python/qemu/machine.py: refactor _qemu_args()

[John Snow]

On 6/2/21 9:10 AM, Vladimir Sementsov-Ogievskiy wrote:
>   - use shorter construction
>   - don't create new dict if not needed
>   - drop extra unpacking key-val arguments
>   - drop extra default values
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>

Sorry, I shuffled around the Python stuff and this doesn't apply 
anymore. I have applied it myself for review, though.

(I don't anticipate any more major shake-ups from this point forward. 
Thanks for your help on reviews for that series.)

> ---
>   python/qemu/machine.py | 18 ++++++++----------
>   1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/python/qemu/machine.py b/python/qemu/machine.py
> index 6e44bda337..ff35f2cd6c 100644
> --- a/python/qemu/machine.py
> +++ b/python/qemu/machine.py
> @@ -541,14 +541,12 @@ def _qmp(self) -> qmp.QEMUMonitorProtocol:
>           return self._qmp_connection
>   
>       @classmethod
> -    def _qmp_args(cls, _conv_keys: bool = True, **args: Any) -> Dict[str, Any]:
> -        qmp_args = dict()
> -        for key, value in args.items():
> -            if _conv_keys:
> -                qmp_args[key.replace('_', '-')] = value
> -            else:
> -                qmp_args[key] = value
> -        return qmp_args
> +    def _qmp_args(cls, conv_keys: bool,
> +                  args: Dict[str, Any]) -> Dict[str, Any]:
> +        if conv_keys:
> +            return {k.replace('_', '-'): v for k, v in args.items()}
> +        else:
> +            return args

qemu/machine/machine.py:558:8: R1705: Unnecessary "else" after "return" 
(no-else-return)

Also, can you try using Dict[str, object] instead? This keeps stricter 
type checking enabled for callers trying to utilize the return value. On 
my review branch (based on master, not kwolf's staging branch) it passes 
Python linting and iotests 297, so it should hopefully not be a hassle.

>   
>       def qmp(self, cmd: str,
>               conv_keys: bool = True,
> @@ -556,7 +554,7 @@ def qmp(self, cmd: str,
>           """
>           Invoke a QMP command and return the response dict
>           """
> -        qmp_args = self._qmp_args(conv_keys, **args)
> +        qmp_args = self._qmp_args(conv_keys, args)
>           return self._qmp.cmd(cmd, args=qmp_args)
>   
>       def command(self, cmd: str,
> @@ -567,7 +565,7 @@ def command(self, cmd: str,
>           On success return the response dict.
>           On failure raise an exception.
>           """
> -        qmp_args = self._qmp_args(conv_keys, **args)
> +        qmp_args = self._qmp_args(conv_keys, args)
>           return self._qmp.command(cmd, **qmp_args)
>   
>       def get_qmp_event(self, wait: bool = False) -> Optional[QMPMessage]:

With those items addressed:

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