Re: [Qemu-devel] [PATCH 5/9] iotests: Different iterator behavior in Python 3

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4588 bytes --]

On 15.10.18 22:07, Eduardo Habkost wrote:
> On Mon, Oct 15, 2018 at 04:14:49PM +0200, Max Reitz wrote:
>> In Python 3, several functions now return iterators instead of lists.
>> This includes range(), items(), map(), and filter().  This means that if
>> we really want a list, we have to wrap those instances with list().  On
>> the other hand, sometimes we do just want an iterator, in which case we
>> have sometimes used xrange() and iteritems() which no longer exist in
>> Python 3.  Just change these calls to be range() and items(), which
>> costs a bit of performance in Python 2, but will do the right thing in
>> Python 3 (which is what is important).
>>
>> In one instance, we only wanted the first instance of the result of a
>> filter() call.  Instead of using next(filter()) which would work only in
>> Python 3, or list(filter())[0] which would work everywhere but is a bit
>> weird, this instance is changed to a single-line for with next() wrapped
>> around, which works both in 2.7 and 3.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> 
> Minor comments below:

[...]

>> diff --git a/tests/qemu-iotests/065 b/tests/qemu-iotests/065
>> index 72aa9707c7..a339bf6069 100755
>> --- a/tests/qemu-iotests/065
>> +++ b/tests/qemu-iotests/065
>> @@ -59,7 +59,7 @@ class TestQemuImgInfo(TestImageInfoSpecific):
>>                      :data.index('')]
>>          for field in data:
>>              self.assertTrue(re.match('^ {4}[^ ]', field) is not None)
>> -        data = map(lambda line: line.strip(), data)
>> +        data = list(map(lambda line: line.strip(), data))
> 
> I would find this more readable:
> 
>         data = [line.strip() for line in data]
> 
> Not a blocker, though.

Yes, I agree, that looks better.

>>          self.assertEqual(data, self.human_compare)
>>  
>>  class TestQMP(TestImageInfoSpecific):
>> @@ -80,7 +80,7 @@ class TestQMP(TestImageInfoSpecific):
>>  
>>      def test_qmp(self):
>>          result = self.vm.qmp('query-block')['return']
>> -        drive = filter(lambda drive: drive['device'] == 'drive0', result)[0]
>> +        drive = next(drive for drive in result if drive['device'] == 'drive0')
> 
> I didn't understand what you meant by "single-line for" on the
> commit message, until I saw the generator expression here.  :)

Maybe I should at least put quotes around the "for", because the
combination of "for with" really makes it difficult to read...

> This will raise an exception if there's no drive0 in the list,
> but that was already true on the original code.
> 
> 
>>          data = drive['inserted']['image']['format-specific']
>>          self.assertEqual(data['type'], iotests.imgfmt)
>>          self.assertEqual(data['data'], self.compare)
>> diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124
>> index 3ea4ac53f5..9f189e3b54 100755
>> --- a/tests/qemu-iotests/124
>> +++ b/tests/qemu-iotests/124
>> @@ -39,7 +39,7 @@ def try_remove(img):
>>  def transaction_action(action, **kwargs):
>>      return {
>>          'type': action,
>> -        'data': dict((k.replace('_', '-'), v) for k, v in kwargs.iteritems())
>> +        'data': dict((k.replace('_', '-'), v) for k, v in kwargs.items())
>>      }
>>  
>>  
>> @@ -134,7 +134,7 @@ class TestIncrementalBackupBase(iotests.QMPTestCase):
>>      def img_create(self, img, fmt=iotests.imgfmt, size='64M',
>>                     parent=None, parentFormat=None, **kwargs):
>>          optargs = []
>> -        for k,v in kwargs.iteritems():
>> +        for k,v in kwargs.items():
>>              optargs = optargs + ['-o', '%s=%s' % (k,v)]
>>          args = ['create', '-f', fmt] + optargs + [img, size]
>>          if parent:
>> diff --git a/tests/qemu-iotests/139 b/tests/qemu-iotests/139
>> index cc7fe337f3..e00f10b8c8 100755
>> --- a/tests/qemu-iotests/139
>> +++ b/tests/qemu-iotests/139
>> @@ -51,7 +51,7 @@ class TestBlockdevDel(iotests.QMPTestCase):
>>      # Check whether a BlockDriverState exists
>>      def checkBlockDriverState(self, node, must_exist = True):
>>          result = self.vm.qmp('query-named-block-nodes')
>> -        nodes = filter(lambda x: x['node-name'] == node, result['return'])
>> +        nodes = list(filter(lambda x: x['node-name'] == node, result['return']))
> 
> I'd prefer a list expression:
> 
>         nodes = [x for x in result['return'] if x['node-name'] == node]
> 
> Also not a blocker.

I don't mind either way, so why not.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]