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

[Jan Cholasta]

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

-- 
Jan Cholasta