[Freeipa-devel] [PATCHES] 0177-0179 Add missing dict methods to CIDict

[Jan Cholasta]

On 23.9.2013 12:08, Petr Viktorin wrote:
> On 09/23/2013 09:18 AM, Jan Cholasta wrote:
>> On 18.9.2013 14:00, Petr Viktorin wrote:
>>> On 09/17/2013 05:13 PM, Jan Cholasta wrote:
>>>> On 20.2.2013 17:37, Petr Viktorin wrote:
>>>>> On 02/19/2013 01:51 PM, Jan Cholasta wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 5.2.2013 18:02, Petr Viktorin wrote:
>>>>>>> CIDict, our case-insensitive dictionary, inherits from dict but did
>>>>>>> not
>>>>>>> reimplement the full dict interface. Calling the missing methods
>>>>>>> silently invoked case-sensitive behavior. Our code seems to avoid
>>>>>>> that,
>>>>>>> but it's a bit of a minefield for new development.
>>>>>>>
>>>>>>> Patch 119 adds the missing dict methods (except
>>>>>>> view{items,keys,values}(), which now raise errors), and adds tests.
>>>>>>
>>>>>> Can you please also add the (obj, **kwargs) and (**kwargs)
>>>>>> variants of
>>>>>> __init__ and update?
>>>>>
>>>>> Added, thanks for the catch.
>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Patches 117-118 modernize the testsuite a bit (these have been
>>>>>>> sitting
>>>>>>> in my queue for a while, I think now is a good time to submit them):
>>>>>>> The first one moves some old tests from the main code tree to
>>>>>>> tests/.
>>>>>>> (The adtrust_install test wasn't run before, this move makes nose
>>>>>>> notice
>>>>>>> it).
>>>>>>> The second converts CIDict's unittest-based suite to nose.
>>>>>>>
>>>>>>
>>>>>> Honza
>>>>>>
>>>>>
>>>>>
>>>>
>>>> Whoa, I totally forgot about these patches!
>>>>
>>>> Can you please rebase them?
>>>
>>> Sure!
>>>
>>>> One more comment:
>>>>
>>>>    Document that CIDict.copy() returns a plain dict.
>>>>
>>>> Why does it return a plain dict? I think it should return a CIDict,
>>>> otherwise it is not actually a copy, right?
>>>
>>> I wanted to keep the existing (intended) functionality.
>>> But since we don't actually use CIDict.copy() anywhere any more, I've
>>> made the change.
>>
>> Thanks.
>>
>>>
>>> P.S. I recently came across a bug in python-ldap where something like
>>> CIDict({'cn': ['name1', 'name2'], 'cN': ['name3']}) will throw away some
>>> of the values.
>>> This is expected at the CIDict level, but if you're working with dicts
>>> of lists it's something to keep an eye out for.
>>>
>>
>> This is a good point. IIRC when you use such a dict in python-ldap, it
>> will fail with TYPE_OR_VALUE_EXISTS. I think we should raise an error in
>> CIDict as well if such a dict is used in __init__() and update(), as
>> this kind of error is very hard to track otherwise.
>>
>> Honza
>>
>
> Right. Here's a patch that does that.
>

Thanks.

ACK to all four patches.

Honza

-- 
Jan Cholasta