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

[Jan Cholasta]

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]

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.

Patch 160:

I think you should do this (replace % with ,):

+            root_logger.debug(
+                "failed to find mapping tree entry for %s", self.suffix)

Honza

-- 
Jan Cholasta