You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@apache.org> on 2016/05/10 14:34:15 UTC

Re: svn commit: r1743195 - /subversion/trunk/subversion/tests/cmdline/svntest/main.py

On 10.05.2016 16:29, stefan2@apache.org wrote:
> Author: stefan2
> Date: Tue May 10 14:29:56 2016
> New Revision: 1743195
>
> URL: http://svn.apache.org/viewvc?rev=1743195&view=rev
> Log:
> Follow-up to r1743183:  Unbreak FSFS svnadmin tests.
>
> * subversion/tests/cmdline/svntest/main.py
>   (ensure_list): Converting strings and bytes to lists is a special case.
>                  Before that, each "element" in them would become a separate
>                  list element.
>
> Modified:
>     subversion/trunk/subversion/tests/cmdline/svntest/main.py
>
> Modified: subversion/trunk/subversion/tests/cmdline/svntest/main.py
> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svntest/main.py?rev=1743195&r1=1743194&r2=1743195&view=diff
> ==============================================================================
> --- subversion/trunk/subversion/tests/cmdline/svntest/main.py (original)
> +++ subversion/trunk/subversion/tests/cmdline/svntest/main.py Tue May 10 14:29:56 2016
> @@ -379,6 +379,8 @@ def ensure_list(item):
>    "If ITEM is not already a list, convert it to a list."
>    if isinstance(item, list):
>      return item
> +  elif isinstance(item, bytes) or isinstance(item, str):
> +    return [ item ]
>    else:
>      return list(item)


I think this is overkill and possibly wrong (and may have been wrong
before, too); the code should probably just be:

    if isinstance(item, list):
      return item
    else:
      return [item]


There's no reason to have a third option, unless you also want to
convert tuples to lists \u2014 but in that case I'd prefer to have an
explicit check for 'isinstance(item, tuple)' instead of explicit checks
for strings and byte sequences.

-- Brane

Re: svn commit: r1743195 - /subversion/trunk/subversion/tests/cmdline/svntest/main.py

Posted by Branko Čibej <br...@apache.org>.
On 10.05.2016 21:11, Stefan Fuhrmann wrote:
> On 10.05.2016 16:34, Branko \u010cibej wrote:
>> On 10.05.2016 16:29, stefan2@apache.org wrote:
>>> Author: stefan2
>>> Date: Tue May 10 14:29:56 2016
>>> New Revision: 1743195
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1743195&view=rev
>>> Log:
>>> Follow-up to r1743183:  Unbreak FSFS svnadmin tests.
>>>
>>> * subversion/tests/cmdline/svntest/main.py
>>>    (ensure_list): Converting strings and bytes to lists is a special
>>> case.
>>>                   Before that, each "element" in them would become a
>>> separate
>>>                   list element.
>>>
>>> Modified:
>>>      subversion/trunk/subversion/tests/cmdline/svntest/main.py
>>>
>>> Modified: subversion/trunk/subversion/tests/cmdline/svntest/main.py
>>> URL:
>>> http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svntest/main.py?rev=1743195&r1=1743194&r2=1743195&view=diff
>>> ==============================================================================
>>>
>>> --- subversion/trunk/subversion/tests/cmdline/svntest/main.py
>>> (original)
>>> +++ subversion/trunk/subversion/tests/cmdline/svntest/main.py Tue
>>> May 10 14:29:56 2016
>>> @@ -379,6 +379,8 @@ def ensure_list(item):
>>>     "If ITEM is not already a list, convert it to a list."
>>>     if isinstance(item, list):
>>>       return item
>>> +  elif isinstance(item, bytes) or isinstance(item, str):
>>> +    return [ item ]
>>>     else:
>>>       return list(item)
>>
>> I think this is overkill and possibly wrong (and may have been wrong
>> before, too); the code should probably just be:
>>
>>      if isinstance(item, list):
>>        return item
>>      else:
>>        return [item]
>>
>>
>> There's no reason to have a third option, unless you also want to
>> convert tuples to lists \u2014 but in that case I'd prefer to have an
>> explicit check for 'isinstance(item, tuple)' instead of explicit checks
>> for strings and byte sequences.
> The list construction is there to convert from iterators
> (e.g. map, filter) to "proper" lists in Python 3. The strings
> special case is mainly carried over from various local
> incarnations of that logic.

Ah ...

I don't think this is limited to Python 3, 2.7 has generators, too. So ack.

> Is there a base class for iterators which is also known
> to Python 2 (doesn't need to be used there) ?

No, iterators and generators don't have a common class hierarchy. In
this the way the code looks now is pretty much the best one can do.

You could mess about with something like this:

    try:
       return list(iter(item))
    except TypeError:
       return [item]


but that will happily consume all strings one byte/char at a time, so
you'd still need an extra condition around it.

-- Brane

Re: svn commit: r1743195 - /subversion/trunk/subversion/tests/cmdline/svntest/main.py

Posted by Stefan Fuhrmann <st...@alice-dsl.de>.
On 10.05.2016 16:34, Branko \u010cibej wrote:
> On 10.05.2016 16:29, stefan2@apache.org wrote:
>> Author: stefan2
>> Date: Tue May 10 14:29:56 2016
>> New Revision: 1743195
>>
>> URL: http://svn.apache.org/viewvc?rev=1743195&view=rev
>> Log:
>> Follow-up to r1743183:  Unbreak FSFS svnadmin tests.
>>
>> * subversion/tests/cmdline/svntest/main.py
>>    (ensure_list): Converting strings and bytes to lists is a special case.
>>                   Before that, each "element" in them would become a separate
>>                   list element.
>>
>> Modified:
>>      subversion/trunk/subversion/tests/cmdline/svntest/main.py
>>
>> Modified: subversion/trunk/subversion/tests/cmdline/svntest/main.py
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/cmdline/svntest/main.py?rev=1743195&r1=1743194&r2=1743195&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/tests/cmdline/svntest/main.py (original)
>> +++ subversion/trunk/subversion/tests/cmdline/svntest/main.py Tue May 10 14:29:56 2016
>> @@ -379,6 +379,8 @@ def ensure_list(item):
>>     "If ITEM is not already a list, convert it to a list."
>>     if isinstance(item, list):
>>       return item
>> +  elif isinstance(item, bytes) or isinstance(item, str):
>> +    return [ item ]
>>     else:
>>       return list(item)
>
> I think this is overkill and possibly wrong (and may have been wrong
> before, too); the code should probably just be:
>
>      if isinstance(item, list):
>        return item
>      else:
>        return [item]
>
>
> There's no reason to have a third option, unless you also want to
> convert tuples to lists \u2014 but in that case I'd prefer to have an
> explicit check for 'isinstance(item, tuple)' instead of explicit checks
> for strings and byte sequences.
The list construction is there to convert from iterators
(e.g. map, filter) to "proper" lists in Python 3. The strings
special case is mainly carried over from various local
incarnations of that logic.

Is there a base class for iterators which is also known
to Python 2 (doesn't need to be used there) ?

-- Stefan^2.