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

[Vladimir Sementsov-Ogievskiy]

03.06.2021 02:38, John Snow wrote:
> 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.

Not a problem, respect and congratulations for final landing of your python 
series!

> (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.

OK

> >       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>

Thanks!

--
Best regards,
Vladimir