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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
