I hid Send by mistake; continuing review:

On 11/08/2013 03:14 PM, Petr Viktorin wrote:
On 10/31/2013 02:45 PM, Jan Cholasta wrote:
Hi,

the attached patches fix <https://fedorahosted.org/freeipa/ticket/3971>.

Tested with 25000 users.

Honza

Patch 198:

Also update ipaldap's find_entries docstring, it no longer uses IPA
defaults.


While you're touching this part of code, I had some other improvements
in mind -- you can consider them:

In find_entries,
     attrs_list = [a.lower() for a in attrs_list]
to make sure 'memberindirect' is case insensitive

In get_memberof, construct `indirect` as a set, for Ο(1) remove().
^ ignore that, it's nuked in 201 \o/

Changing MEMBERS_ALL et.al. from numbers to descriptive strings, for
easier debugging.
^ these can be removed entirely in 201



Patch 199: Looks great


Patch 200:

objtype, res_list, red_id, res_ctrls = result
Minor typo ----------^


This construction won't work as you'd expect in Python 2:

try:
     (possibly raise interesting exception) (*)
except:
     try:
         (possibly raise exception to ignore) (**)
     except:
         pass
     raise  # (***)

The problem is that the exception in (**) overwrites the "current active
exception" raised in (*). In (***) the exception from the cleanup will be
re-raised.
The solution is to store the wanted exception info, including the
traceback:
     exc_type, exc_value, exc_traceback = sys.exc_info()
and then re-raise explicitly:
     raise exc_type, exc_value, exc_traceback

Also, please log the ignored exception from cancelling the paged search.




Patch 201:
Great patch!
A nitpick, I'd rename _process_member{,of} to _process_member{,of}indirect


Patch 202: Looks good
While we're on the subject: Each Plugin has an "api" attribute. It would be nice if we started preferring `self.api` instead of the global singleton wherever possible, even though they're currently always the same.


--
Petr³

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

Reply via email to