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

[Petr Viktorin]

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.

-- 
Petr³

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0280.3-Detect-updating-CIDict-with-duplicate-keys.patch
Type: text/x-patch
Size: 3917 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130923/b29a3e7f/attachment.bin>