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

Petr Viktorin pviktori at redhat.com
Wed Sep 25 08:15:03 UTC 2013


On 09/24/2013 12:03 PM, Jan Cholasta wrote:
> 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.
>

Thanks, pushed to master: a93fc02af6eb50ecb0cfc69174c9f385d60bbbb3

-- 
Petr³




More information about the Freeipa-devel mailing list