From: Gerald B. <ger...@gm...> - 2010-05-23 18:48:17
|
Good work on finding that bug and adding the needed functionality in living.py! Interestingly, in Python 3.0 or even 2.6 if one imports the new-style map function: for handle in self.plist: yield self.get_person_from_handle(handle) can become from future_builtins import map # or from itertools import imap as map return map(self.get_person_from_handle, self.plist) I find this a nice simplification! I must get around to applying this in trunk. The other think I'm wondering about is if self.plist, self.flist etc need to be lists. If they were sets some things would speed up nicely and some logic would become set logic. The question is, is the order in which handles are added to these lists important? If so, we need to leave things as they are; if not, we can make them sets. Out of caution, I left them as lists for 3.2, but I'm wondering again about making them sets. On Sun, May 23, 2010 at 8:32 AM, Doug Blank <dou...@gm...> wrote: > On Sun, May 23, 2010 at 8:21 AM, Gerald Britton > <ger...@gm...> wrote: >> On Sun, May 23, 2010 at 7:54 AM, Doug Blank <dou...@gm...> wrote: >>> On Sun, May 23, 2010 at 7:38 AM, Gerald Britton >>> <ger...@gm...> wrote: >>>> On Sun, May 23, 2010 at 1:18 AM, Doug Blank <dou...@gm...> wrote: >>>>> Gerald, >>>>> >>>>> I believe that I fixed a bug in the filter proxy code [1] and added >>>>> three missing methods in the living proxy class [2]. Please let me >>>>> know if those look like appropriate fixes. Thanks, >>>>> >>>>> -Doug >>>>> >>>>> PS - I found these by playing around with a way to visualize the proxies. >>>>> >>>>> [1] - http://gramps.svn.sourceforge.net/viewvc/gramps?view=rev&revision=15451 >>>> >>>> There is something wrong with this update. The original code is just: >>>> >>>> def iter_person_handles(self): >>>> """ >>>> Return an iterator over database handles, one handle for each Person in >>>> the database. >>>> """ >>>> return self.plist >>>> >>>> which I believe to be correct. However the diff refers to 15450, >>>> which I cannot see in SVN at all. From what I can see, the original >>>> code (above) is correct, unless you can provide a fail scenario >>> >>> Gerald, >>> >>> Change #15451 refers to: >>> >>> http://gramps.svn.sourceforge.net/viewvc/gramps/trunk/src/gen/proxy/filter.py?r1=15451&r2=15450&pathrev=15451 >>> >>> which is about iter_people. It was: >>> >>> for handle in self.plist: >>> yield handle, self.get_person_from_handle(handle) >>> >>> which does not match what gen.db does. >> >> Not, that's not the original. The original just returns self.plist, >> which I think is correct behavior. I cannot see where the for-loop >> was introduced. > > Looking at the annotated version (I'm in trunk): > > http://gramps.svn.sourceforge.net/viewvc/gramps/trunk/src/gen/proxy/filter.py?annotate=15451&pathrev=15451 > > it looks like you added that 13140. If plist is a list of handles, > then iter_people should return people, and get_person_from_handle > should do the correct thing. > >> The previous rev (13676), is just: >> >> def iter_person_handles(self): >> >> return self.plist >> >> I believe that this is correct as is and do not know where the code >> came from that your rev is based on. Clearly the code your rev is >> based on is incorrect, but it differs from the original and is itself >> incorrect. >> >>> >>>>> [2] - http://gramps.svn.sourceforge.net/viewvc/gramps?view=rev&revision=15453 >>>> >>>> This should not be needed. The method include_person provides the >>>> test needed and the standard methods in ProxyBase call this method >>>> before including data. >>>> >>>> Can you provide a fail scenario? >>> >>> These changes need to have the self.__restrict_person(person) applied >>> in the iterators. Nothing does that in the ProxyBase, correct? >> >> I see. You want to do post processing of the data before returning it. >> In that case, you need iter_people but not iter_person_handles nor >> get_person_handles. Note that __restrict_person always returns a >> Person object. > > Ok, thanks. > > -Doug > >>> >>>> Also, this is considered bad form: >>>> >>>> handles = [] >>>> 136 for handle in self.db.get_person_handles(): >>>> 137 person = self.get_person_from_handle(handle) >>>> 138 if person: >>>> 139 handles.append(handle) >>>> >>>> You should always use a list generator instead: >>>> >>>> handles = list(handle for handle in self.db.iter_person_handles() >>>> if self.get_person_from_handle(handle)) >>> >>> Yes, nicely written. >>> >>>> but, as I said above, I do not believe that this is needed at all. >>> >>> Let me know... I don't see the living proxy applied if I iterate. >>> >>> -Doug >>> >>>>> >>>> >>>> >>>> >>>> -- >>>> Gerald Britton >>>> >>> >> >> >> >> -- >> Gerald Britton >> > -- Gerald Britton |