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 <mre...@redhat.com> > > Reviewed-by: Eduardo Habkost <ehabk...@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
signature.asc
Description: OpenPGP digital signature
