[Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)

[Jan Cholasta]

On 20.2.2013 13:03, Petr Viktorin wrote:
> On 02/19/2013 03:10 PM, Jan Cholasta wrote:
>> On 1.2.2013 15:38, Petr Viktorin wrote:
>>> Alright, I renamed get_single to single_value().
>>>
>>> I also rebased to current master.
>>>
>>
>> Patch 152:
>>
>> +    def single_value(self, name, default=_missing):
>> +        values = self.get(name, [default])
>> +        if len(values) != 1:
>> +            raise ValueError(
>> +                '%s has %s values, one expected' % (name, len(values)))
>> +        if values[0] is _missing:
>> +            raise KeyError(name)
>> +        return values[0]
>>
>> I would prefer if you used __getitem__() instead of get() and re-raised
>> KeyError if default is _missing. Also, until we disallow non-lists, we
>> need to check if values actually is list. I.e., do something like this:
>>
>> +    def single_value(self, name, default=_missing):
>> +        try:
>> +            values = super(LDAPEntry,
>> self).__getitem__(self._get_attr_name(name))
>> +        except KeyError:
>> +            if default is _missing:
>> +                raise
>> +            return default
>> +        if not isinstance(values, list):
>> +            return values
>> +        if len(values) != 1:
>> +            raise ValueError(
>> +                '%s has %s values, one expected' % (name, len(values)))
>> +        return values[0]
>
> Updated, thanks.
>
> _get_attr_name is only added in your patch 99, so I used _attr_name here
> and modified your patch.
>
> I wrote some tests for single_value, and while I was at it I added tests
> for a few other LDAPEntry methods as well. I've also split things up
> into more testcases. Including as patch 0181.

Thanks. I think you should also add a tearDown method to test_LDAPEntry 
which disconnects self.conn if it is connected (the same thing test_ldap 
does).

>
>> Patch 159:
>>
>> Like I said in my review of your patch 143, I think we should use DNs
>> instead of entries in delete_entry, so I think it would make sense to do
>> delete_entry(entry.dn) in this patch.
>
> I think that for symmetry with add_entry and update_entry, delete_entry
> should take entries too. If you already have the entry, having to type
> the extra ".dn" is not very intuitive.
> The proper thing to do would be a separate delete_by_dn(), but
> delete_entry() taking either entries or DNs works fine IMO.

OK, makes sense.

>
>> Patch 160:
>>
>> I think you should do this (replace % with ,):
>>
>> +            root_logger.debug(
>> +                "failed to find mapping tree entry for %s", self.suffix)
>>
>
> Fixed, thanks.
>
> I've also rebased 174 and your patch 104 to current master.
>

Honza

-- 
Jan Cholasta