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

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to