[Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)
Petr Viktorin
pviktori at redhat.com
Wed Feb 20 12:03:27 UTC 2013
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.
> 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.
> 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.
--
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0152-03-Introduce-LDAPEntry.single_value-for-getting-single-.patch
Type: text/x-patch
Size: 1851 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130220/428b3991/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0160-02-Fix-typo-and-traceback-suppression-in-replication.py.patch
Type: text/x-patch
Size: 1172 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130220/428b3991/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0174-03-Remove-some-uses-of-raw-python-ldap.patch
Type: text/x-patch
Size: 37062 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130220/428b3991/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-104pviktori-Remove-DN-normalization-from-the-baseldap-plugin.patch
Type: text/x-patch
Size: 14615 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130220/428b3991/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-99pviktori-Preserve-case-of-attribute-names-in-LDAPEntry.patch
Type: text/x-patch
Size: 10565 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130220/428b3991/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0181-Improve-LDAPEntry-tests.patch
Type: text/x-patch
Size: 4950 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130220/428b3991/attachment-0005.bin>
More information about the Freeipa-devel
mailing list