#launchpad-reviews 2010-01-04
<jpds> stub, jml: Thanks for the db review.
<jpds> noodles775: Ping.
<noodles775_> Hi jpds
<jpds> noodles775_: Hi there, when you have time, I think I need a UI review for https://code.edge.launchpad.net/~jpds/launchpad/fix_361650/+merge/16749 .
<noodles775_> jpds: great! I'll take a look in a bit.
<jpds> Awesome, thanks.
* al-maisan changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [jpds, al-maisan<http://tinyurl.com/yhj8h7l>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [jpds, al-maisan<http://tinyurl.com/yhj8h7l>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> jpds: hi! Can you add some details about testing your MP? (ie. steps to demo the new functionality, who to log in as, a url to access etc.).
<noodles775> Just for next time perhaps...
<jpds> noodles775: Just a sec.
<noodles775> jpds: i've already looked at the code, so np this time. It's just helpful to provide them in the MP.
<jpds> noodles775: OK, I'll add the details next time.
<noodles775> jpds: what's the reasoning behind not doing this as an admin-only field displayed on the normal edit form?
* adiroiban changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [jpds, al-maisan<http://tinyurl.com/yhj8h7l>, adiroiban(bug-340662)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> adiroiban: what's the branch? have you already arranged to have someone review it, or would you like me to look at it now?
<adiroiban> https://code.edge.launchpad.net/~adiroiban/launchpad/bug-340662/+merge/16629
<adiroiban> intellectronica: hm... I did not arranged any review. Just created a MP and add it to the topic, to let you know it needs review
<adiroiban> this is how it went for my previous branches
<adiroiban> were there any changes in the MP process?
<intellectronica> adiroiban: cool, i'll review it
<al-maisan> intellectronica: could you please also review the branch I queued (http://tinyurl.com/yhj8h7l) when you get to it?
<intellectronica> al-maisan: sure, will look at it as soon as i finish adiroiban's branch
<al-maisan> intellectronica: thanking you :)
<jpds> noodles775: Do you mean the +review form?
<noodles775> jpds: no, I meant the +edit form (but i'm logged in as an admin, maybe that's not presented for mirror-admins...). Checking now.
<jpds> noodles775: I wanted to make it a little obvious by having the separate button for it.
<intellectronica> adiroiban: looks fine to me. i will run the tests and land on your behalf if it passes cleanly
<adiroiban> intellectronica: thanks. Let me know if there are any errors. Before creating the MP I did a full test for translations module
<jpds> noodles775: And mirror registrants shouldn't be able to see the checkbox themselves.
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [jpds, al-maisan<http://tinyurl.com/yhj8h7l>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> al-maisan: i must break for lunch, but will review your branch as soon as i'm back
<al-maisan> intellectronica: enjoy your lunch!
* salgado changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [jpds, al-maisan<http://tinyurl.com/yhj8h7l>,salgado] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [jpds, al-maisan<http://tinyurl.com/yhj8h7l>,salgado, mthaddon] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi intellectronica, can I add mthaddon's branch to the queue? https://code.edge.launchpad.net/~mthaddon/launchpad/buildjob-nagios-check-perms/+merge/16532
<intellectronica> noodles775: sure
<intellectronica> al-maisan: can you please add a comment before the then-block on line 33 of the diff? just to distinguish it from the condition
<al-maisan> intellectronica: sure, no problem.
<intellectronica> al-maisan: cool. r=me, everything else looks flawless
<al-maisan> intellectronica: thank you very much!
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue [jpds,salgado, mthaddon] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: -,- || queue [jpds,salgado, mthaddon] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> jpds: Does your change break any existing pagetests?
<jpds> abentley: No.
<jpds> abentley: They change the message with ... .
<jpds> abentley: check*
<abentley> jpds: In that case, what would you think about adding a pagetest that ensures the correct text and link is used?
<abentley> jpds: I see that the bug is not triaged.  Have you discussed your change with anyone on the bugs team?
<jpds> abentley: No, I filed it during the holidays.
<abentley> deryck: jpds has a branch fixing bug #499997, which is untriaged.  Do you agree it should be fixed?
<mup> Bug #499997: Duplicate warning box should link to duplicate bug report <Launchpad Bugs:In Progress by jpds> <https://launchpad.net/bugs/499997>
 * deryck looks
<deryck> abentley, yup, a link would be nice.  I'll mark it accordingly triaged.
<abentley> deryck: thanks.
<abentley> jpds: What would you think about adding a pagetest that ensures the correct text and link is used?
<jpds> abentley: I'm looking for the test now to add that.
<jpds> abentley: Pushed revision with link test.
<abentley> jpds: Thanks!  Looking...
<noodles775> sinzui: I've just added you as a ui reviewer for jpds' branch - I hope that's ok.
<abentley> jpds: r=me.  Has this branch passed the full test suite?
<jpds> abentley: For bugs, yes. That took a while to run...
<sinzui> noodles775: yes
<abentley> jpds: When it has passed the full test suite, I can land it for you.  We don't land anything that hasn't passed the full test suite.
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: -,- || queue [salgado, mthaddon] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> salgado-lunch: r=me
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: -, mthaddon || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: salgado, mthaddon || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> intellectronica: I just reviewed that.
<intellectronica> ah, cool
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: -, mthaddon || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* intellectronica changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> abentley: can you review a branch for me? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part3/+merge/16804
<abentley> EdwinGrubbs: Sure, just a minute.
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue [EdwinGrubbs] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: EdwinGrubbs || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> EdwinGrubbs: What is the new behavior for addMember?
<abentley> EdwinGrubbs: It is a bit strange to see the names of James Blackwell and Jeff Waugh.  They are former Canonical people, so I'd expect to see them only in old tests.
<abentley> What would you think about getting foo.bar by email rather than 'name16'?
<EdwinGrubbs> abentley: I'm fine with switching it to get foobar by email rather than name16.
<abentley> EdwinGrubbs: I think that would make it a bit easier to understand.
<EdwinGrubbs> abentley: addMember() now takes into account whether the user is an admin of both teams. If they are, there is no reason to make them go through the invite/accept-invitation dance. Just add the member as active.
<abentley> EdwinGrubbs: Cool.
<abentley> EdwinGrubbs: The behaviour of inTeam is not what I would expect.
<EdwinGrubbs> abentley: Well, I decided to use James Blackwell, etc, since I'm trying to avoid a part4 of this branch.
<abentley> EdwinGrubbs: I would expect that (x.inTeam(y)) == (y in x.allmembers)
<abentley> EdwinGrubbs: Sorry, that should be (y.inTeam(x)) == (y in x.allmembers)
<EdwinGrubbs> abentley: can you tell me in which file you are looking at inTeam()?
<abentley> EdwinGrubbs: I am looking at your diff for lib/lp/registry/doc/teammembership.txt
<abentley> EdwinGrubbs: http://pastebin.ubuntu.com/351412/
<EdwinGrubbs> abentley: yes, that is just the bizarro behavior, where the owner of a team can leave the team but still be the owner. I don't know why we let that happen to begin with, but that is too big of an issue to tackle here.
<EdwinGrubbs> abentley: inTeam() is used by the security layer, so that's why they don't have it match currently.
<abentley> EdwinGrubbs: It's the behaviour of inTeam in that circumstance that's surprising, not so much the fact that the circumstance can arise.  inTeam is used by our security stuff, too.  We may have to reconsider this.
<leonardr> salgado, i made a change to my anonymous-oauth branch back on december 16th. can you take a quick look at it and change your vote from 'needs fixing' so i can land it?
<salgado> leonardr, will do.  do you have the URL of the m-p?  btw, did you add a comment to the m-p after you made the changes?  I rely heavily on email to do reviews, so commenting on the m-p when there's changes made to the branch is a good idea
<leonardr> salgado: https://code.edge.launchpad.net/~leonardr/launchpad/anonymous-oauth/+merge/16199
<leonardr> i didn't leave a comment. i probably got the branch cleared with gary since i remember him agreeing to land it for me, but since that was a long time ago i want to finish it properly
<salgado> leonardr, do you have an incremental diff?  I really don't want to go through the whole thing all over again
<leonardr> salgado: check out revisions 10558 and 10559...
<leonardr> hm, launchpad isn't cooperating
<leonardr> salgado: here you go
<leonardr> https://pastebin.canonical.com/26172/
<salgado> thanks leonardr 
<salgado> leonardr, just updated the m-p; r=me
<abentley> EdwinGrubbs: Is the doc correct?  It looks inverted: http://pastebin.ubuntu.com/351422/
<bac> hi abentley - can you review another?  https://code.edge.launchpad.net/~bac/launchpad/bug-498179/+merge/16806
<EdwinGrubbs> abentley: The doc is incorrect. Thanks for catching that.
<abentley> bac: I will try, but the one I'm currently reviewing is large.
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: EdwinGrubbs || queue [bac] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> abentley: ok.  trying is good.
<EdwinGrubbs> bac: I can review that one for you.
<bac> EdwinGrubbs: hey, that's great
<abentley> EdwinGrubbs: Wow, doctests sure punish us for adding useful return values!
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> EdwinGrubbs: r=me
<EdwinGrubbs> abentley: thanks alot
<EdwinGrubbs> abentley: I started reviewing Brad's branch, in case you didn't see that message.
<abentley> EdwinGrubbs: I did, thanks.
<EdwinGrubbs> bac: What is the purpose of adding the pub_member to the pub_team? It appears to have no effect on the test.
<bac> EdwinGrubbs: likely a mistake
<bac> EdwinGrubbs: oh, i was going to show that even though pub_owner could see private parts the mere pub_member could not
<bac> EdwinGrubbs: i think that's still a good thing to do, i just forgot to do it
<bac> EdwinGrubbs: this is what i intended:  http://pastebin.ubuntu.com/351429/
<EdwinGrubbs> that looks good
<EdwinGrubbs> bac: I was thinking about the test to see if the user is in getDirectAdministrators(), and it seems like it would cause a problem for teams that are owned by another team. Therefore, you should probably use can_edit_team(invited_team, user)
<bac> EdwinGrubbs: good idea, i think
<EdwinGrubbs> bac: review sent
<bac> EdwinGrubbs: thanks
<bac> EdwinGrubbs: what do you think about this instead?  http://pastebin.ubuntu.com/351446  it avoids some redundant checking for admins that would happen if i used can_edit_team
* abentley changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-01-05
<jtv> jml: you're looking at my db patch as well?  If so, tia :)
<jml> jtv, np.
<thumper> jml: ping
<jml> thumper, pong.
<thumper> jml: re: removeSecurityProxy
<thumper> jml: there is a big comment above as to why we need to change the datecreated
<thumper> jml: are you explicitly mentining the use of the proxy in this case?
<jml> thumper, in this case, yes.
<thumper> jml: the member isn't publicly accessable, hence the need
<thumper> I'll add a comment
<jml> thumper, I'm remembering having this conversation about changing the datecreated, actually :)
<jml> thumper, why can't we make it accessible?
 * thumper thinks
<thumper> no reason really I guess
<thumper> it is readonly at the moment
<thumper> normally there is no need to modify the default value
<thumper> this is a special case
<thumper> I'd rather not expose it for a special case
<thumper> that is all really
<jml> hmm.
<jml> BjornT, are you around?
<BjornT> hi jml 
<thumper> BjornT: Karma.datecreated is currently readonly (like most datecreateds)
<jml> BjornT, ... and we want to backdate karma
<thumper> BjornT: do we change it to read/write for the special case where we want to backdate karma
<thumper> BjornT: or use removeSecurityProxy?
<jml> see allocateKarma in lp.code.model.revision
<jml> https://code.edge.launchpad.net/~thumper/launchpad/revision-karma-fix/+merge/16825 for context
<thumper> jml: well, it currently doesn't need the proxy removed, as it isn't wrapped at the point of access
<thumper> but by change to use the target to get it wraps it
<jml> *nod*
<BjornT> thumper, jml: without looking, it sounds like you should try to specify the right datecreated when creating the karma in the first place. would that be possible?
<jml> thumper, was just thinking that BjornT might want to look at the code to get some context as to _why_ we are fiddling w/ datecreated.
<jml> hmm. maybe.
<thumper> BjornT: not through the person.allocateKarma method
 * jml looks.
<thumper> we could add yet another parameter I guess
<thumper> assignKarma that is
<jml> thumper, yeah, that's what I was thinking.
<jml> thumper, that'd probably be nicer.
<jml> thumper, and wouldn't make the branch much bigger at all -- it's three lines of code
<thumper> jml: exept for the tests to the change to assignKarma
<jml> thumper, I'm not going to make you test it :P
<jml> thumper, if you make it an optional parameter, you won't have to change any of the existing tests.
<thumper> yeah...
<thumper> almost all params to assignKarma are optional
<jml> l/reg/doc/person-karma.txt are tests, I think.
<thumper> jml: slightly more than 3 lines: 5 files changed, 20 insertions(+), 12 deletions(-)
<jml> thumper, heh
<jml> thumper, well, I'm not in coding any more, I'm in product management -- everything ought to be simple, right?
 * jml recalls an account manager in a past job who insisted that every feature was only ever a weekend's work
<jml> I should try not to be that guy
<jml> anyway, people are putting drinks and delicious food in front of me -- good night.
<allenap> gmb: When you have a mo, could you look at the post-review changes in https://code.edge.launchpad.net/~allenap/launchpad/checkwatches-stuff/+merge/16495?
<allenap> bac: It's an old one, from before the hols, but when you have time can you have a look at the incremental changes I made to https://code.edge.launchpad.net/~allenap/launchpad/remove-xmlrpclib-marshalling-bug-254999/+merge/16436?
<gmb> allenap: Looking now.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> allenap: You need to remove lib/lp/bugs/scripts/tests/test_checkwatches.py.moved
<allenap> gmb: Gah, I hate computers.
<gmb> allenap: Other than that, r=me.
<allenap> gmb: Thanks.
* gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> gmb: In rev 10054.7.1, there's a method NonConnectingBugzillaAPI.getProductsForRemoteBugs that refers to a bug_ids variable that doesn't exist. Things seem to be working anyway, so do you think it might be safe to drop that method?
<allenap> gmb: Sorry, rev 10054.7.1 is where it was added.
<gmb> allenap: I have no idea... Drop it and see what happens. Is it in test_checkwatches.py?
<allenap> gmb: Yep.
<gmb> allenap: Then it's part of a regression test... Let me take a look.
<allenap> gmb: It runs without it.
<gmb> allenap: Then it should be safe to drop it.
<allenap> gmb: Cool.
* gmb changed the topic of #launchpad-reviews to: on call: gmb[lunch] || reviewing: sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* jpds changed the topic of #launchpad-reviews to: on call: gmb[lunch] || reviewing: sinzui || queue [jpds(361650)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on call: gmb[lunch], bac || reviewing: sinzui || queue [jpds(361650)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> good morning
<bac> hi allenap, i'll have a look at your MP shortly
* bac changed the topic of #launchpad-reviews to: on call: gmb[lunch], bac || reviewing: sinzui, allenap || queue [jpds(361650)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> allenap: scratch that.  i see gmb already did.
* bac changed the topic of #launchpad-reviews to: on call: gmb[lunch], bac || reviewing: sinzui, jpds(361650) || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: sinzui, jpds(361650) || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> Morning bac.
<bac> hi gmb -- how's the frostbite?
<allenap> bac: Hi Brad. Graham looked at a different mp, so my request still stands.
<allenap> bac: If that's okay?
<bac> allenap: ok, but i've already started on another.  will get to yours shortly.
<allenap> bac: Brilliant, thanks.
<gmb> bac: I'll survive. Perfectly pleasant 3-mile walk for milk this morning and all my extremities still in working order.
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: sinzui, allenap|| queue [jpds(361650) ] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> allenap: donning your former-build-engineer hat, do you know why 'ec2 land' would not submit to PQM after a successful test run?  at the end of the email is "successfully ran all tests" but no message about submitting to PQM.
<allenap> bac: I don't know off the top of my head, but I'll take a look.
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: mwhudson, allenap || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> bac: ec2 land invokes ec2 test -s "message", so I can only think that the test runner failed after running tests, or didn't pick up a correct email configuration from your ~/.bazaar configuration.
<bac> allenap: that would be odd.  it worked before the break and my config hasn't changed.
<bac> allenap: i wonder if there is a trap for import_violations > METRIC_SHITLOAD
<allenap> bac: Heh, yes, maybe :)
<sinzui> The import violations are trivial fixes for the most part
* gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, allenap || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> allenap: on your bug 254999 branch is the only difference from the last review that you fixed more files?  just more of the same?
<allenap> bac: Yes, more of the same.
<bac> allenap: looks good.  r=bac
<allenap> bac: Thanks :)
<allenap> bac: I can't find anything that causes the test suite to go boom if there are anti-fascist imports, but perhaps the fascist's atexit handler itself blew up... seems unlikely though.
<bac> allenap: ok, thanks for looking
<sinzui> gmb: regarding the unclear error message in delete team: I wanted to it to be: I am sorry Dave, but I cannot let you craft an application to delete other people's data and gain control of their projects.
<sinzui> gmb: I am not sure what to write given that the the message is to someone trying to break into Launchpad
<gmb> Hmm.
<gmb> sinzui: Gimme a sec, just on a call; I'll have a think.
<gmb> sinzui: So, something simple like "unable to process submitted data"  - given this is an edge case and a nasty one at that we don't have to be terribly explicit. If you add a comment to explain why you're being vague that would work fine for me.
<sinzui> okay
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> jtv: hi. sorry for the delay. I was as bit bussy today
<jtv> hi adiroiban!  I'm about to leave (in about -1.5 hours to be exact :)
<adiroiban> np. tomorrow is also a day 
<adiroiban> i got stuck with modifying the 2 configure.zcml
* BjornT changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, - || queue [BjornT] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<BjornT> gmb, bac: can anyone please review https://code.edge.launchpad.net/~bjornt/launchpad/enable-windmill-again?
<bac> BjornT: i will
<BjornT> thanks
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, BjornT || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> abentley: hi. is there anything I need to do for https://code.edge.launchpad.net/~adiroiban/launchpad/bug-359180/+merge/16422 ?
<abentley> adiroiban: Has it passed the full test suite?
<adiroiban> abentley: I only did the full test for translations module
<adiroiban> my computer is low on RAM and I can not do a full test
<adiroiban> as I get an out of memory error
<adiroiban> in soyuz 
<abentley> adiroiban: We can't land it until it passes the test suite.  Currently, I'm having trouble running the full suite myself.
<abentley> adiroiban: perhaps gmb or bac would be willing to run it through ec2 for you.
<gmb> adiroiban, abentley: I'll take care of it.
<adiroiban> abentley: ok. for the same reason I am stuck with https://code.edge.launchpad.net/~adiroiban/launchpad/bug-496352-series-status-refactor/+merge/16108
<gmb> adiroiban: I'll do that, too :)
<abentley> gmb: thanks.
<abentley> gmb: x2
<adiroiban> gmb: for  the series-status-refactor let me do a new merge 
<gmb> Welcome ^2
<adiroiban> as it will probably fail after merging with devel 
<adiroiban> the branch is pretty old and there is a chance that someone has added a new DistroSeriesStatus 
<adiroiban> this will be merged without conflicts
<adiroiban> but since it was renamed to SeriesStatus
<adiroiban> the merged branch will be broken
<gmb> adiroiban: Sure. Just ping me when ready.
<jpds> gmb: Can you also ec2 test https://code.edge.launchpad.net/~jpds/launchpad/fix_499997 for me?
<gmb> jpds: Sure
<gmb> jpds: Running
<jpds> gmb: Thanks.
* bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> hi all, please to be reviewing https://code.edge.launchpad.net/~james-w/launchpad/sync-source-negative-versions/+merge/16857
<james_w> gmb: ^ a bit of soyuz to warm your cockles?
<gmb> Eurgh.
<gmb> james_w: But since it's you.
<james_w> heh
* gmb changed the topic of #launchpad-reviews to: gmb, bac || reviewing: james_w, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> oh, did I get the target branch wrong?
<adiroiban> gmb: both branches should are ready for a full test. Thanks@
<gmb> adiroiban: Okidoke. Will set them off shortly.
<adiroiban> gmb: no hurry
<gmb> WTF: bzr: ERROR: xmlrpc protocol error connecting to https://xmlrpc.edge.launchpad.net/bazaar/: 302 Found
<gmb> Hmm.
<gmb> adiroiban: Er, https://code.edge.launchpad.net/~adiroiban/launchpad/bug-496352-series-status-refactor/+merge/16108 appears to be 2000+ lines long and has 4 (apparently) unreviewed revisions in it. Can you take a look to see exactly what's gone on there please?
<adiroiban> gmb: it is a rename of DistroSeriesStatus to SeriesStatus
<gmb> adiroiban: Ah, so it was always 2000+ lines?
<adiroiban> gmb: the new revisions are just fixing problems after merge
<adiroiban> gmb: yes
<gmb> adiroiban: Okay, thanks.
<gmb> james_w: Your change to security.py seems not relevant to the branch (I admit, I've not looked further down the diff yet). Is it?
<james_w> gmb: it seems I started from the wrong branch/proposed for the wrong branch
<gmb> Oh.
<gmb> That's a new one.
<james_w> I worked from devel, what should I propose merge in to?
<gmb> Oh, I see.
<gmb> james_w: launchpad/devel
<gmb> lp:launchpad is actually db-devel
<gmb> For stacking purposes.
<gmb> james_w: want me to reject that m-p?
<james_w> I just deleted
<james_w> https://code.edge.launchpad.net/~james-w/launchpad/sync-source-negative-versions/+merge/16861
 * gmb looks
<gmb> james_w: There appear to be no tests for the change. I don't know Soyuz well enough to know whether there *can't* be any tests or not... can there?
<gmb> (I'm aware here that you might know the same amount as me on this front)
<james_w> <james_w> where would I find the tests for ./scripts/ftpmaster-tools/sync-source.py ?
<james_w> <bigjools> james_w: hahahaha
<james_w> gmb: so I think no tests is the status quo
<gmb> Hah.
<gmb> Right.
<gmb> james_w: In that case, r=me.
<james_w> would you be so kind?
<gmb> james_w: Sure.
<james_w> I don't have PQM rights
<gmb> Fair dos.
<james_w> thanks gmb 
* gmb changed the topic of #launchpad-reviews to: on-call: bac || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi bac - are you ok for a non-interactive review? (I'll be off in a few mins):
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/499095-build-retry-depwait-stuck/+merge/16865
* sinzui changed the topic of #launchpad-reviews to: on-call: bac || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> noodles775: sure
<noodles775> Thanks.
<bac> sinzui: why do you silently put things in the queue?  like pavlov's dog i only respond when i hear the bell.
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: - || queue [sinzui noodles] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> bac: because I have a cat dropping books on me
<sinzui> and it is now on meee and clawwinnng me
<bac> good reason, i guess
<bac> i could loan you jojo for a day and take care of that problem
<sinzui> I have seen jojo try to get in your lap. I do not think that is much help
<bac> sinzui: thanks for showing a lplib test in your MP
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: sinzui || queue [noodles] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> bac: I have a whole script if you want
<bac> sinzui: nah, i just usually have to nag people to test API changes with lplib
<sinzui> bac: EdwinGrubbs pointed out that I had mis-spelled the parameters in the webservice test. That is what I have been trying to fix since 2:30 PM EST yesterday. I suck
<bac> sinzui: spelling is not your strongest point
<sinzui> It will be my demise.
<sinzui> abentley: rockstar: I want to resubmit a merge proposal to make it clear that it should be merged into db-devel, not devel. If I do so, will I loose all the excellent conversation and reviews that the proposal already has?
<rockstar> sinzui, I don't think you can resubmit against another branch.
 * rockstar checks
<abentley> sinzui: You can't resubmit with a different target branch.  And if you could, the excellent conversation would not be shown on the new proposal.  We are discussing changing both these things.
<sinzui> abentley: thanks for your insight. I do not think this is a common issue. jml/stuart should have caught the error when they started the review.
* sinzui changed the topic of #launchpad-reviews to: on-call: bac || reviewing: sinzui, jpds || queue [noodles] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> bac: to be clear, I am reviewing jpds's branch
<EdwinGrubbs> salgado: can you mark this mp as approved? I fixed the last two issues you listed, but I hadn't notice that the status on your review was still needs-fixing. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-482176-add-team-member-ajax-part2/+merge/16325
<salgado> EdwinGrubbs, sure
<salgado> EdwinGrubbs, what did you do with the code that was commented out?
<EdwinGrubbs> salgado: I removed it entirely. It's just a message, and we normally show those with an info icon and no "Notice" header above it.
<bac> sinzui: when i try to access findPerson(...) i get "Unrecognized parameters 'created_before' and 'created_after'"
<bac> sinzui: this using your lplib snippet
<bac> sinzui: what have i done wrong?  i rebuilt the wadl and removed my lplib cache file
<sinzui> interesting
<sinzui> I had to 
<sinzui> rm lib/canonical/launchpad/apidoc/wadl-development.xml; make lib/canonical/launchpad/apidoc/wadl-development.xml
<sinzui> bac ^ which is what I think make build does when the wadl does not exist
 * bac tries
 * bac fails
<sinzui> bac: is there any chance you are not using env=dev in your test script?
<sinzui> I can send you my test script
<bac> sinzui: i'm definitely hitting dev.  i see the responses in the server window
<sinzui> make clean; make build ?
<bac> done
<sinzui> do you see created_after in lib/canonical/launchpad/apidoc/wadl-development.xml ?
<bac> sinzui: figured it out
<sinzui> please tell
<bac> the cached wadl is *NOT* in api.launchpad.dev/cache but is in api.launchpad.dev/
<bac> sinzui: so, now that i can play with it in lplib -- it works very well
<bac> sinzui: r=bac
<sinzui> thanks
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: noodles || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: on-call: bac || reviewing: jtv || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> mwhudson, can you do the review for this code-import-list branch?  I think it'll make you giggle.
<mwhudson> rockstar: sure
<mwhudson> rockstar: will i be embarrassed too?
<rockstar> mwhudson, you shouldn't be.
<rockstar> mwhudson, sending now.
<mwhudson> cool
<rockstar> argh, forgot to sign me email.
<rockstar> One day, I'll be able to use my mind to verify my identity.
<mwhudson> rockstar: aaaaaaaaaaaargh!
<rockstar> mwhudson, :)
<rockstar> When I found it, I kind of chortled/facepalmed
<rockstar> I was expecting it to be really complicated.
<rockstar> thumper, could I get you to look at https://code.edge.launchpad.net/~rockstar/launchpad/branch-scanner-prep/+merge/16260 again?  I have some failing tests for the next branch, and then I can land these both.
<thumper> rockstar: yes I'll look
<rockstar> thumper, and as far as the <= is concerned, yes, I know it doesn't make sense.  It seems to be a storm oddity.  I had it the other way and couldn't get the test to pass, and then I noticed another garbo method had it that way, and changing it passed the test.
* bac changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-01-06
<rockstar> thumper, ping.
<thumper> rockstar: hey hey
<rockstar> thumper, unping.  I need to check to make sure my assertion is correct before making it.
<rockstar> thumper, NOW I think I'm ready to land this branch.  I've taken out the tree creation (at one point I was running the job, so it needed a tree)
<thumper> rockstar: is it pushed?
<rockstar> thumper, yessir.  A bit ago (I got distracted)
<thumper> rockstar: https://code.launchpad.net/~thumper/launchpad/more-product-series-permissions/+merge/16888
<rockstar> thumper, wait, I have to scratch your back for to scratch mine?  :)
<thumper> rockstar: you don't *have* to...
<rockstar> thumper, I'm kidding.  Looking now.
<rockstar> thumper, so this allows us to delete import branches that are the series branch?
<thumper> rockstar: yes
<rockstar> Awesomeness.
<thumper> rockstar: now I have a few questions
<rockstar> thumper, shoot.
<thumper> rockstar: why create two branches in the test?
<thumper> rockstar: if you are only testing that newer ones aren't deleted, you don't need two branches
<thumper> branch jobs
<thumper> just one
<thumper> is enough
<rockstar> Hm, okay.
<thumper> then you can remove the first assert
<thumper> also why use transaction.commit()
<thumper> is that really necessary>
<thumper> ?
<thumper> you have no comment at the top of the test
<thumper> you have two lines between the tests
<rockstar> thumper, yes, otherwise it blows up apparently.
<thumper> and I don't think you need tempfile anymore
<thumper> apparently or certainly?
<rockstar> thumper, it was blowing up otherwise.
<thumper> with what error?
<thumper> I don't see why you should need the commit
<rockstar> thumper, lemme try.
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/revision-karma-fix/+merge/16890
<thumper> mwhudson: ta, using critical
<rockstar> thumper, yea, looks like the transaction.commit is needed.
<thumper> rockstar: hmm
<thumper> ok
<thumper> rockstar: have you fixed the other bits?
<rockstar> thumper, no idea, but the other tests in the TestCase do it as well.
<rockstar> thumper, other bits fixed, pushing now.
<thumper> ok
<rockstar> thumper, diff is updated.
<thumper> rockstar: replied, a few more things to fix
<rockstar> thumper, I think I got them all this time.
 * rockstar REALLY wants to send this branch off the ec2
<rockstar> thumper, going home from the coffee shop, back soon (hopefully to submit to PQM)
<thumper> ok
<thumper> rockstar: you are good to go now
<rockstar> thumper, sweet, thanks.
* jtv changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> stub: I followed up on your db review yesterday... is it enough to take you from Needs Information to Approved?
<stub> jtv: updated
<jtv> ta
<al-maisan> hello jtv, are you still reviewing?
<jtv> al-maisan: indeed!
<al-maisan> Great! Would you mind taking a look at https://code.edge.launchpad.net/~al-maisan/launchpad/xx-next-builder-502927/+merge/16896 ?
<jtv> gimme
<jtv> looking...
<al-maisan> thanks :)
<jtv> oh nice... will this one also generalize BuildQueue.required_build_behavior?
<jtv> seems no... but progress is progress :)
<al-maisan> jtv: not really .. this is still about job dispatch time estimation
<al-maisan> step n-3
<jtv> Oh, I see you got builder_key un-nested... I like it better this way
<jtv> (but didn't say anything last time because IIRC you had nested it based on another reviewer's suggestion)
<al-maisan> yes, it was jml's suggestion I believe
<al-maisan> now builder_key() is used in more than one place
<jtv> damn his morning croissant
<al-maisan> .. and the crumbles :)
<jtv> But where is _minTimeToNextBuilderAvailable actually called?
<al-maisan> jtv: nowhere yet
<jtv> (tests aside, of course)
<al-maisan> that will come in one of the next branches
<jtv> OK
<jtv> Then bear in mind that a method name should begin with a verb
<al-maisan> the branch this is taken from is almost 2K LOC
<al-maisan> ah .. a verb
<jtv> maybe "estimate"?
<al-maisan> I can fix that
<al-maisan> "estimate" is nice .. I like that.
<jtv> instead of "min" (because the docstring suggests this is an estimated time, not a mininum)
<al-maisan> good idea!
<jtv> Also, "Get" is one of those over-generic and over-used words...  Why say "Get estimated time" when you're really producing one?  Same verb can serve there.  :-)
<al-maisan> hmm .. fair enough.
<jtv> Also, mixed_mode is a bit cryptic.  Can you document it in the docstring?
<al-maisan> jtv: I can but the big comment at the beginning of the method is all about the "mixed mode"
<jtv> al-maisan: I look at the comments when I want to know more about the implementation, but the docstring should give me the "This Method for Dummies" meaning of the parameter.
<jtv> How about processor_dependent?
<jtv> or even, is_processor_specific?
<jtv> As the law goes, two things are hard in computer science: cache invalidation and naming things.
<al-maisan> jtv: I can add the explanation in the doc string.
<al-maisan> .. and yeah, naming things is not easy, I agree :)
<al-maisan> "mixed_mode" really expresses the meaning best IMHO
<al-maisan> think about a queue with jobs
<al-maisan> what this method needs to know is whether there are jobs ahead of the job of interest (JOI) that are processor independent
<al-maisan> if yes, then all builders should be considered
<jtv> isn't that "mode" a property of the BuildQueue object?
<al-maisan> jtv: not quite each job has a BuildQueue
<jtv> Right, that's why I'm asking.
<al-maisan> BuildQueue should actually be named BuildQueueJob
<jtv> The BuildQueue's job can be architecture-specific or non-architecture specific... isn't that what determines which value will be passed here?
<al-maisan> what is being passed in here is: are there any jobs ahead of me that are processor independent?
<al-maisan> ahead of me in the sense that they have a higher score and ..
<al-maisan> .. should be dispatched before me
<al-maisan> me being the build queue job of interest
<jtv> al-maisan: maybe it would help me to understand why you need the two "modes."  Are you trying to get an estimate of when this particular job can start running (assuming that it's at a front of the queue)?
<al-maisan> again, think of a queue of jobs
<al-maisan> jtv: please give me few minutes to come up with a clarifying example
<jtv> al-maisan: it would help me a lot right now to have an idea of what this method will be used for
<al-maisan> OK
<al-maisan> the question this method answers is: what is the shorted time until a builder capable of running the JOI becomes available?
<al-maisan> jtv: s/shorted/shortest/
<jtv> but then my previous question stands: isn't whether the job is architecture-dependent or not a property of the BuildQueue object (or its associated Job etc.)?
<al-maisan> yes, that's what's encoded in the 'my_processor' local variable
<al-maisan> but
<al-maisan> there's (at least) one other scenario where that is not sufficient
<al-maisan> job queue [amd_job_1, proc_independent_job_2, x86_job_3, x86_job_4]
<al-maisan> let's assume x86_job_4 is the JOI
<al-maisan> in that case proc_independent_job_2 and x86_job_3 compete for its builder pool
<al-maisan> but not amd_job_1
<al-maisan> so, what I need to know is: how long is the delay until the head of the queue of mutually competing jobs can be dispatched
<jtv> (btw otp sorry)
<al-maisan> jtv: actually, I just realized that the method may not be doing what it's supposed to do.
<al-maisan> jtv: please ignore this merge proposal until I come back to you.
 * jtv finally gets off phone
<jtv> al-maisan: the entire MP?  And we were still on the first line of diff!
<jtv> new record for me ;-)
<al-maisan> jtv: you are asking good questions :)
<jtv> "I could tell from the way his first two characters rendered in my browser that there was a bug somewhere"
<jtv> meanwhile, I'll go have a standup :)
<al-maisan> jtv: thanks for looking at this anyway :)
<jtv> no worries, it's what we're working on!
<al-maisan> hello gmb, I know it's not your OCR day but I have a branch that needs to land today. Would you be in a position to take a look at it? It's 611 lines long but the bulk of it are tests.
<al-maisan> here's the diff: http://pastebin.ubuntu.com/352336/
<gmb> al-maisan: Sure.
<gmb> al-maisan: Is there a merge-proposal for this?
<al-maisan> gmb: I really appreciate your help .. the MP is here: https://code.edge.launchpad.net/~al-maisan/launchpad/xx-next-builder-502927/+merge/16896
<gmb> al-maisan: No worries :). I'll grab a cup of tea and then take a look.
<al-maisan> gmb: if you want to go through examples, I have a picture of some scenarios here: https://devpad.canonical.com/~muharem/IMG_0275.JPG
<gmb> al-maisan: Thanks.
<al-maisan> we can also have a voice call .. whatever suits you.
<gmb> al-maisan: Bear with me; I'll ping you when I need more information :)
<al-maisan> gmb: great :)
* abentley changed the topic of #launchpad-reviews to: on-call: jtv || reviewing: - || queue [abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> al-maisan: So, what happens if our 120 second wild guess is wrong? Anything bad?
<al-maisan> gmb: not really .. the estimate will not be as good as we'd like.
<al-maisan> the dispatch time estimate that is
<gmb> al-maisan: Okay, cool. I think that the comment in the SQL should be updated to reflect that it's not going to make horrible things happen.
<al-maisan> gmb: fair enough, can do that.
<gmb> al-maisan: Very impressive branch. r=me with the comment change and one other minor nitpick (see review)
<al-maisan> gmb: thanks a million! I owe you one :)
<gmb> :)
* EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: jtv, Edwin || reviewing: - || queue [abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> EdwinGrubbs: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/no-review-diff/+merge/16900 ?
<EdwinGrubbs> abentley: sure
<abentley> EdwinGrubbs: Thanks.
<abentley> EdwinGrubbs: jtv is listed in the topic, but apparently has connectivity issues.
<leonardr> flacoste, i'm getting somewhat close to being able to land the first major multiversion branch. i wonder if you're interested in going over it with me. it would be a fairly major project (i have divided it into two chunks but each chunk is larger than 800 lines)
<leonardr> (ie. i am trying to find someone to do a review, and i thought of you since you're interested in this project)
<flacoste> leonardr: yes, i'd have time for this this afternoon
<leonardr> flacoste, ok, great
<EdwinGrubbs> abentley the docstring for test_preview_diff_text_with_no_diff() says that the review_diff should be None when there is no context.preview_diff. Is that right? I don't really understand the difference between a review diff and a preview diff or whether all the objects treat them as separate.
<abentley> EdwinGrubbs: The confusion between review diffs and preview diffs was one of the reasons we got rid of review diffs.  A review diff shows the changes the branch author made.  A preview diff shows what would happen if you merged that branch.
<abentley> EdwinGrubbs: The review_diff field on the view class now actually refers to the preview diff, so the docstring is accurate.
<EdwinGrubbs> abentley: is that first docstring in the diff correct, or should "review_diff" be "preview_diff". In the test you are checking view.preview_diff_text.
<abentley> EdwinGrubbs: I think review_diff should be preview_diff_text.
<EdwinGrubbs> abentley: The docstring for test_preview_diff_utf8 doesn't make any sense.
<abentley> EdwinGrubbs: It means that if the bytes are utf-8-encoded, they should be decoded to unicode as utf-8.
<EdwinGrubbs> abentley: can you clarify that in the docstring?
<abentley> EdwinGrubbs: "A preview_diff in utf-8 should be decoded as utf-8" ?
<EdwinGrubbs> abentley: yes, but there is no reason to say utf-8 twice, can you say "decoded into a unicode object"?
<abentley> EdwinGrubbs: The reason to say it twice is because it should not be decoded using a different encoding.
<abentley> e.g. "A preview_diff in utf-8 should be decoded as utf-8, not utf-16"
<EdwinGrubbs> abentley: ok, that makes sense, since that's what you are really trying to test.
<EdwinGrubbs> abentley: in xx-branch-merge-proposals.txt, a tag with id='review-diff' is retrieved. Is that just a case where the UI hasn't yet been updated to match the name of the attribute that is now used?
<abentley> EdwinGrubbs: Yes.
<EdwinGrubbs> r=me with the two docstring changes
<abentley> EdwinGrubbs: Thanks.
<leonardr> flacoste, let me know when you're ready to start the review
<flacoste> send it to me, i'll start on it after lunch
<leonardr> ok, the first bit is some code i pushed back in november
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/version-specific-request-interface/+merge/15172
<leonardr> i'm working on writing up the subsequent changes
<leonardr> i tried to use looms for this branch but i didn't do a very good job
<leonardr> on top of that branch, i've got one tiny thread and one enormous thread
<flacoste> leonardr: i'll look at that m-p after lunch and contact you if i have any questions
<leonardr> ok
<leonardr> then we'll move from there
<flacoste> EdwinGrubbs: can I have r=you for adding a top-level .ctags to the launchpad tree?
<EdwinGrubbs> flacoste: sure
<EdwinGrubbs> flacoste: thanks for doing that
<flacoste> leonardr: around?
<leonardr> flacoste, yes
<flacoste> leonardr: do you remember why in multiversion.txt you have to register an IComponentLookup adapter?
<flacoste> everything_uses_the_global_site_manager
<leonardr> flacoste: i was modeling it after webservice.txt. i assumed it was because the test was running in a very minimal environment
<leonardr> i'll try removing it and seeing if it still works
<leonardr> flacoste, that code is unnecessary both in webservice.txt and multiversion.txt. i'l remove it from both places
<flacoste> leonardr: ok, thanks!
<flacoste> leonardr: what is the use of IVersionedClientRequestImplementation
<flacoste> ?
<flacoste> I'm reading the "Defining the request interfaces" section
<flacoste> and the narrative doesn't talk about that interface
<leonardr> flacoste: IVCRI makes it easy to look up a marker interface based on the version string
<flacoste> leonardr: ok, you probably want to explain that in the narrative then
<leonardr> sure
<flacoste> leonardr: first review done
<leonardr> flacoste, ok
<leonardr> because the original branch causes a huge number of test failures (the second branch is all about fixing these), i'm going to make your suggested changes to the second branch
<flacoste> leonardr: ok, i won't have time to review the second branch today though
<leonardr> flacoste: no problem
<leonardr> i'm almost done for the day. i'll spend the rest of the day responding to your review and we can pick up tomorrow
<leonardr> flacoste: not sure why those conflict markers are showing up
<leonardr> i'll re-push my branch and hopefully they'll disappear
<flacoste> leonardr: branch trunk and merge your branch
<flacoste> see if you get any conflicts
<leonardr> nope
#launchpad-reviews 2010-01-07
<stub> https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/16898
<al-maisan> Hello there! Is anybody reviewing?
<al-maisan> I have a pretty interesting and short branch for review: https://code.edge.launchpad.net/~al-maisan/launchpad/spec-job-class-503839/+merge/16946
<al-maisan> jml: would you be in a position to take a look at the branch above?
<jml> al-maisan, sorry, not right now.
<al-maisan> jml: no problem :)
* al-maisan changed the topic of #launchpad-reviews to: on-call: jtv, Edwin || reviewing: - || queue [abentley, al-maisan<http://tinyurl.com/yelblmx>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* al-maisan changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [abentley, al-maisan<http://tinyurl.com/yelblmx>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: - || queue [abentley, al-maisan<http://tinyurl.com/yelblmx>] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: al-maisan || queue [abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> Alwight noodles775, can I add myself to the queue?
<noodles775> allenap: of course :)
* allenap changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: al-maisan || queue [abentley, allenap] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> noodles775: Thanks :)
* noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: allenap || queue [abentley] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Sorry allenap, I just finished al-maisan's branch (as I got involved in a few other things). Starting now.
<allenap> noodles775: Cool, thanks.
<allenap> noodles775: I'm actually going for lunch now, so please skip to someone else if you want. Having said that, my branch is big but a simple refactoring; I don't think it's controversial.
<noodles775> Yeah, I don't mind doing it all on the MP
<allenap> noodles775: Thanks.
<allenap> noodles775: Thanks for the review. Good catch on the wording!
<noodles775> np!
* noodles775 changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> fjlacoste, whenever you're ready, i've fixed up multiversion-collection and it's ready for review
* rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
 * rockstar goes to get his hairs cut
#launchpad-reviews 2010-01-08
<jml> mwhudson, while you're still around, would you like to review my import warnings branch?
<mwhudson> jml: definitely
<jml> mwhudson, https://code.edge.launchpad.net/~jml/launchpad/fix-import-warnings/+merge/16965
<mwhudson> jml: i wonder why those files imported things from _schema_circular_imports
<jml> mwhudson, me too.
<mwhudson> that just seems a bit odd
<jml> mwhudson, my guess is that the module moving thingy did it
<jml> mwhudson, I guess I could 'bzr blame'
<mwhudson> jml: "+ # The bug turned to be security-related, subscribe the security"
<mwhudson> jml: can you insert the missing "out" ?
<mwhudson> it's in lib/lp/bugs/subscribers/bug.py
<jml> mwhudson, done.
<mwhudson_> jml: can you paste the link again
<jml> <jml> mwhudson, https://code.edge.launchpad.net/~jml/launchpad/fix-import-warnings/+merge/16965
<mwhudson_> ta
<jml> last lines of convo...
<jml> <mwhudson> it's in lib/lp/bugs/subscribers/bug.py
<jml> <jml> mwhudson, done.
<mwhudson_> i actually saw those
<mwhudson_> jml: i guess i should ask you to file a bug for those XXXs but i can forget to if you like
<mwhudson_> otherwise, very much r=me
<jml> mwhudson, can you please change some status on the MP?
<mwhudson> ah right yes
<jml> mwhudson, thanks.
<jml> mwhudson, another branch for you
<mwhudson> jml: ok
<jml> https://code.edge.launchpad.net/~jml/launchpad/upcall-testcase/+merge/17002
<mwhudson> jml: BRANCH.TODO still has quite a lot in it
<jml> mwhudson, oops
<jml> mwhudson, fixed
<mwhudson> jml: it's almost like you have some automatic way of finding pep8 violations
<jml> mwhudson, it's uncanny that you should mention it, because in fact it turns I out I have.
<mwhudson> jml: also i guess that doing the super().setUp() style when you pass the user argument is theoretically a bit risky
<jml> mwhudson, blame python
<mwhudson> yeah
<mwhudson> jml: approved
<jml> mwhudson, thanks.
* rockstar changed the topic of #launchpad-reviews to: on-call:  || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<stub> https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/16898
<jml> stub, replied.
<stub> Fixing now. Ta.
<stub> jml: pushing now. Diff at https://pastebin.canonical.com/26350/ for the impatient. 
<jml> stub, it's spelled "separate".
<jml> stub, other than that, r=jml
<stub> Not when I spell it is isn't ;)
 * stub needs misspelling highlighting in vim
<jml> there was a terrible awful mnemonic on the wall in one of my classes in primary school
<jml> smell "a rat" when you sep_arat_e
* adeuring changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> Hi adeuring! Bock auf'n review? ;-)
* henninge changed the topic of #launchpad-reviews to: on-call: adeuring || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> adeuring: didn't even see that you'd already reviewed my import policy violations fix... thanks!
<adeuring> jtv: welcome :)
<adeuring> henninge: sorry, didnt notice your request... Yes, I can of course do the review
<henninge> adeuring: np ;)
<henninge> thanks
* salgado changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: henninge, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> salgado: i just submitted a MP for a branch to fix bug 419930.  can you review it?
<mup> Bug #419930: non-public email address shown after merge request <email> <merge> <Launchpad Registry:Triaged> <https://launchpad.net/bugs/419930>
<salgado> bac, sure!
<bac> salgado: cool b/c you're the best one to do it.  thanks.
<henninge> adeuring: Hi, how are you doing with the review? Any questions?
<adeuring> henninge: I have one suggestion:
<bac> salgado: it just arrived: https://code.edge.launchpad.net/~bac/launchpad/bug-419930/+merge/17015
<adeuring> henninge: you list the the "SQL names" o the person objects in ILaunchpadCelebrities in l/c/l/doc/celebrities.txt. What do you think of listing instead the _attribute_ names of IPerson objects in ILauchpadCelebrities()?
* bac changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: henninge, - || queue [bac] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> The latter is what you need in IPersonRoles
<henninge> adeuring: yes, correct. This was the cheapest way to do it because the SQL names were already being collected.
<adeuring> henninge: yes. But you can iterate over ILaunchpadCelebrities.names() to find those celebrities that implement IPerson.
<henninge> adeuring: actually, I'd check for PersonCelebrityDescriptor
<adeuring> henninge: OOPS, right
<henninge> adeuring: but you're right, that shouldn't be too hard.
<henninge> thanks for the suggestion.
<adeuring> henninge: Then you could also get rid of the person_names attribute in ILPCelebs
<adeuring> henninge: other that that; i found a few typos. I'll send the nitpicks via mail ;)
<henninge> adeuring: how? I still need a way to get to the list of names - be they sql names or attribute names.
<adeuring> henninge: right. You already use IPersonRoles.names() in a test; ILPCelebrities.names() should give you all celebrity names; and with IPersonCelebrity.providedBy(getattr(all_celebrities, attribute_name) you can check if you have a "relevant" celeb object, I think
<henninge> adeuring: no doubt, but I want to do that in a test and to get to that in a test, it needs to be in an interface somewhere.
<henninge> adeuring: PersonCelebrityDescriptor is not exported and I don't want to do it just for the test.
<adeuring> henninge: OK, makes sense. Wat about adding a class attribute is_person_celebrity to CelebrityDescriptor and PersonCelebrityDescriptor?
<adeuring> s/wt/what/
<henninge> adeuring: I had thought about that but thought I could achieve the same thing just using isinstance (eventually I resorted to just counting).
<henninge> adeuring: but if I do that *instead* of person_names, it makes more sense.
<henninge> adeuring: I'll do that.
<adeuring> henninge: OK, but there is even an attribute CelebrityDescrptor.interface. COuldn't you use that one?
<henninge> and check for IPersonSet? possibly
<henninge> hm
<adeuring> henninge: right, that's a bit odd
<henninge> adeuring: no, I think it's odder to add an attribute to a base class that is meant for one specific child class
<henninge> which is_person_descriptor would be
<adeuring> henninge: right...
* salgado changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: henninge, bac || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> adeuring: problem is, ILaunchpadCelebrities has attributes that are no descriptors at all
<henninge> adeuring: so I'd have to check for existence of the "interface" attribute first, then check its value.
<adeuring> henninge: right. But: reading the documentation of __get__() again, I think you can simply use IPerson.providedBy(celebrity)
<henninge> oh, really? let me try that.
<henninge> adeuring: ok, that works but I had to go through "getattr" for the Descritor to be invoked. I tried to loop through __dict__ first but that does not invoke __get__. And it looks ugly.
<henninge> again something learned about Python 
<adeuring> henninge: yeah, getattr() was what I was thinking about ;)
<henninge> adeuring: look http://paste.ubuntu.com/353497/
<adeuring> henninge: looks good. But I think it you should also explicitly print the names of the "person celebrities", so that you see more clearly that you should change IPersonRoles when you add/remove/change an IPerson celebrity. (sorry to be such PITA...)
<henninge> adeuring: np
<adeuring> henninge: Or perhaps you could even create a sorted list of the person celeb names, prefixed with "in_" and compare that list with the corresoding list of attributes from IPersonRoles
<henninge> adeuring: that is what I am doing now.
<adeuring> henninge: cool!
<henninge> just the other way round, I am removing the prefix from the IPersonRoles attribute names.
<adeuring> henninge: yeah, sure ;)
<henninge> adeuring: I meant, that is what the patch does.
<adeuring> ok
<adeuring> henninge: OK, now I get it; what I meant is: The comparison in line 64 is http://paste.ubuntu.com/353497/ is fine. But when something goes wrong, you need to figure out what goes wrong. It helps a little bit if you print also the content of person_celebrity_names
<henninge> adeuring: working on that atm, using set
<adeuring> henninge: great, thanks! otherwise, r=me, BTW
<henninge> adeuring: cool, thank you!
* adeuring changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: -, bac || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> bac, sorry for the delay on that review
* salgado changed the topic of #launchpad-reviews to: on-call: adeuring,salgado || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> salgado: np
<bac> salgado: to answer your question, if a duplicate person has hidden email addresses then you can merge them if you know the LP id.  is that problematic?
<salgado> bac, no, I was just wondering if it's currently possible to merge a dupe account with hidden email addresses
<bac> salgado: you cannot get the person via the picker but you can enter the id in the field
<salgado> right
<bac> thanks for catching the missing test and the suggested improvements
<bac> salgado: fancy another?
<salgado> bac, sure, why not?
<bac> salgado: https://code.edge.launchpad.net/~bac/launchpad/bug-407604-proj-desc/+merge/17028
<bac> salgado: i am about to leave for lunch and an appt so i won't be around to answer questions on IRC
<salgado> that's fine
* adeuring changed the topic of #launchpad-reviews to: on-call: salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> EdwinGrubbs: hey.  So, I'm still thinking about the vocabulary thing you pinged me about.  Even though I don't feel "done," I figure I'd share my thoughts so you could steer me or maybe provide an idea I would have missed.
<gary_poster> To start trivially and less problematically (for me at least), I think lines 177-180 should be a "try: ... except UnauthorizedError: ..." rather than looking for launchpad.View.  IMO, you should never have to ask for a given permission.  
<gary_poster> (FWIW, zope.security.checker.canView would be better because it does what you want without having to specify a permission, and would incidentally Just Work without the secured vocabulary work you have done, but I agree with Francis that a security proxy is a better approach.)
<gary_poster> EdwinGrubbs: huh
<gary_poster> EdwinGrubbs: did you get everything I just wrote?  It ended with "lines 138-148, written in zcml for each vocabulary definition?"
<EdwinGrubbs> gary_poster: No, I didn't see that line.
<gary_poster> EdwinGrubbs: what was the last line you got?
<gary_poster> before I said "huh"
<gary_poster> (if any)
<EdwinGrubbs> gary_poster: the last line I got ended with "security proxy is a better approach.)"
<EdwinGrubbs> gary_poster: can I use canView() instead of the try/except UnauthorizedError?
<gary_poster> EdwinGrubbs: you could.  Why do you want to?
<gary_poster> When we finish this I guess I'll paste in the next item :-)
<EdwinGrubbs> gary_poster: well, because exception handling is slow, and because I have seen canView() used elsewhere in launchpad but exception handling for permissions not so much.
* salgado changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> EdwinGrubbs: slow: I'm skeptical.  I know the advice of which you speak, but I doubt exception handling is slow compared to calling another Python function.  I'd want to see a timeit test before that was a valid argument.  I think exception handling is slow compared to other single-Python-operation tasks.  
<gary_poster> exception handling for permissions: huh.  Of course, that's the principle for Zope's basic operation.  I think try: except is more appropriate here.  I'm OK with you using canView for now, and letting me bring this up in the reviewers meetings (are we even having those now that barry is gone?) if that's what you want.
<gary_poster> I'll copy and paste my next point then.
<gary_poster> Still staying easy, my next topic is the way you handled __getslice__ for ICountableIterator.  I prefer what you have done rather than modifying ICountableIterator, if I understand the situation properly.  Am I correct that this is necessary because Python will try __getslice__ before trying __getitem__?  If so, this is clearly a security issue, and the scribbling belongs where you have it, IMO.
<gary_poster> If you are OK with that, I'll then copy and paste my last bits, which was where I wrote quite a lot.
<gary_poster> well, more than the bit you read, anyway.
<EdwinGrubbs> gary_poster: bac has taken over the reviewer meetings. I guess you missed the one on Wed.
<gary_poster> EdwinGrubbs: cool.  Yeah, I was in Lean training with the OEM folks, since I wasn't with LP the first time around.
<EdwinGrubbs> gary_poster: excuses, excuses
<gary_poster> heh
<EdwinGrubbs> gary_poster: __getslice__ is used because HugeVocabularyJSONView batches the results.
<gary_poster> EdwinGrubbs: but the implementation handles slices in its __getitem__, right?
<gary_poster> I mean, the vocabulary implementation
<EdwinGrubbs> gary_poster: right
<gary_poster> EdwinGribbs: cool.  Then are you OK with me being OK with what you have done there? :-)
<EdwinGrubbs> gary_poster: as far as try/except for security exceptions. I have no problem with it, if that is preferred, but I can imagine getting questioned on it in the code review, so it would be good to bring it up at the next meeting.
<gary_poster> EdwinGrubbs: cool, makes sense
<EdwinGrubbs> gary_poster: yes, I approve of your approval. I guess I'm ready for the pasted text.
<gary_poster> OK cool
<gary_poster> Five chunks
<gary_poster> Finally, the bigger and harder question is the first one you raised, about creating a new vocabularyutility directive rather than hacking securedutility. Honestly, I don't love either approach. 
<gary_poster> The securedutility approach is a kludge, as you said.  If we were going to pursue it, I'd like to try doing it a slightly different way, but we can talk about that later if necessary.  (Summary: register another directive in first phase of zcml processing rather than just doing it in the second stage.)
<gary_poster> The vocabularyutility directive introduces yet another zcml directive, when I think that in practice one of zcml's failings is that it was never actually designed as a language but as a collection of automations.  (If I'm wrong, and the original intent was a language, then I think the intent was lost.)  Therefore, I don't want that one either.
<gary_poster> I tend to think that what we need is to look at the problem you are trying to solve and step back.  We can introduce a new zcml directive, but if we do, I'd rather it be of more general usefulness.  It might not be possible, or possible within a short timeframe, but I still think we need to talk about it.
<gary_poster> The problem you were trying to solve was "a ton of extra boiler plate".  I think I know what that boiler plate was, but do you have a full example, or can you easily construct it?  Am I right that it is lines 138-148, written in zcml for each vocabulary definition?
<gary_poster> That's the end of the paste.
<gary_poster> I'm pretty sure that the last para is right about lines 138-148.  I still don't have an answer I like. :-(
<gary_poster> That is, I've tried to "step back" and I still don't see anything...
<gary_poster> I believe the boiler plate is something on the order of this:
<gary_poster> <allow interface="...IVocabularyFactory" /><allow interface="...IHugeVocabulary" />
<EdwinGrubbs> gary_poster: exactly
<EdwinGrubbs> gary_poster: If we don't want the zcml directive doing the work. One alternative that may or may not be considered even more kludgy would be to post process the registered utilities. Of course, I have absolutely no idea how to go about doing that.
<EdwinGrubbs> gary_poster: I'm not sure what you meant about the first and second phase of zcml processing.
<gary_poster> EdwinGrubbs: zcml processing has two rounds.  First, it parses the XML and registers directive classes with unique identifiers.  If it encounters a directive with the same identifier already registered, it pukes, and announces a conflict.  This is how zcml makes sure that two directives don't fight (or even duplicate) one another.  If the zcml processing gets through this phase, then it actually *performs* the zcml direc
<gary_poster> In the case of your code, registering directives for the classes would make sure that you are not stomping on anything else.  It also would have caught the problem that made you use the _vocabulary_cache...though you might still have needed that solution, in order to avoid complaints.
<gary_poster> EdwinGrubbs: OK after thinking about it maybe more than I should have, here's my opinion, which you might not like :-) .  I think you should make an interface that combines IVocabularyFactory and IHugeFactory into a single interface for convenience, and do <allow interface="...combined interface" /> for now.
<gary_poster> Solving this is a matter for thought--and in fact, this is the kind of thought that the grok community has been pursuing.  That project is about reducing Zope boiler plate (and making it easier to start with Zope 3 technologies) and so I think we will want to use their approach for simplifying this kind of thing in the future.
<gary_poster> For now, I don't think we should introduce a surprising kludge or Yet Another ZCML directive.
<gary_poster> That's my take.
<EdwinGrubbs> gary_poster: I just realized that I was wrong when I said that the boilerplate I was eliminating were just the two <allow> directives. I can't combine those two directives with a new interface, since one <allow> will be in <securedutility> and the other will be in <class>. See http://pastebin.ubuntu.com/353609/
<EdwinGrubbs> gary_poster: I'm not sure how grok would make this situation better. Wouldn't require just as many extra configurations, or is it considered less tabu to create custom configuration directives, since it doesn't have the difficulty of trying to figure out where a zcml directive is actually defined or what it does?
<EdwinGrubbs> gary_poster: it really seems like we need something like the <pages><page/></pages> hierarchical directives, so we can fill in redundant attributes. However, it wouldn't be quite as simple since there is both a <securedutility> and <class> directive to fill in.
<gary_poster> EdwinGrubbs: So, first, for the "allow" concern, here's a reply: http://pastebin.ubuntu.com/353615/
<gary_poster> EdwinGrubbs: oh...
<gary_poster> EdwinGrubbs: you need to treat the IVocabularyFactory as something that the context provides
<gary_poster> EdwinGrubbs: and you need to treat the resulting class as something that the context implements
<gary_poster> EdwinGrubbs: that's why my idea doesn't work
<gary_poster> I mean "you need to treat the IHugeVocabulary as something that the context implements"
<EdwinGrubbs> gary_poster: yes, in lines 138-148, I had to use a separate ClassDirective instead of piling everything into self.allow()
<gary_poster> EdwinGrubbs: grok better: I think you would be able to define the utility in a single line, and define the permission for the class in a single line.  It would not compress the two behaviors the way you have it here, but I suspect it would be both more readable/explicit and shorter.  (Moreover, if grok does not have support for secured utilities, we could add it.)
<gary_poster> admittedly, you would still have to follow the "recipe" for a secured vocabulary, which is your point.
<EdwinGrubbs> gary_poster: zcml seems anti-DRY. Maybe I could create a new paradigm, WET (We exemplify tediousness).
<EdwinGrubbs> ;-)
<gary_poster> lol
<EdwinGrubbs> gary_poster: despite my negativity, if you think that being explicit in the zcml is the best way to go, I'll do that.
<gary_poster> EdwinGrubbs: "convention over configuration": "convention" usually is not explicit.  configuration typically does not conform to DRY.  grok is about trying to get some of the zope benefits while bringing in some DRY ideas.  
<gary_poster> I'm not against your concerns, I'm just against trying to address them in a way that introduces another issue IMO (makes the zcml "language" bigger with more to learn) without some significant pushback.  I reconciled my position in my own mind by thinking "grok is the better way to reduce duplication".
<gary_poster> I'll summarize my position like this:
<gary_poster> 1) My primary vote is to just grin and bear it, and wait for grok and/or some deeper thought on how to reduce DRY in our code.
<gary_poster> 2) If you feel strongly that you don't want to wait for some unspecified time in the future for this, I'm uncomfortably OK with you making a securedvocabulary (or <vocabulary secure="true" />?) directive.
<gary_poster> 3) I'd really rather not see the kludge in securedutility.
<gary_poster> EdwinGrubbs: fair enough?  I need to run go pick up a son from school
<gary_poster> EdwinGrubbs: back in a bit
<gary_poster> '
<EdwinGrubbs> gary_poster: go ahead, I'm getting distracted in another channel.
<gary_poster> EdwinGrubbs: back fwiw
<gary_poster> EdwinGrubbs2: I
<gary_poster> have been back for a while.  don't know if you saw that (I see you are having nick fun)
<EdwinGrubbs2> gary_poster: I saw your 1/2/3 summary. It's ok. sed should make it easy enough.
<gary_poster> EdwinGrubbs: cool
<sinzui> flacoste: I replied with a two remarks. Do you want to change the security on featured projects? Do you want to run the doc tests on the DatabaseFunctionalLayer?
#launchpad-reviews 2010-01-10
<thumper> noodles775: https://code.edge.launchpad.net/~thumper/launchpad/lines-right-aligned/+merge/17109
#launchpad-reviews 2011-01-03
* bac changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> jcsackett: i know this isn't your OCR day but care to do a review?
<jcsackett> bac: sure.
<bac> jcsackett: thanks.  https://code.launchpad.net/~bac/launchpad/bug-693357/+merge/45075
<jcsackett> bac: any chance you could also take a look at one for me?
<bac> jcsackett: sure
<jcsackett> cool. https://code.launchpad.net/~jcsackett/launchpad/mergeproposal-status-icon-589584/+merge/44531
<jcsackett> thanks, bac.
<jcsackett> bac, are there any tests for get PPAOwnedByPerson?
<bac> jcsackett: just those in the doc test
<jcsackett> bac: okay. soyuz is nasty enough i'm not going to suggest trying to build new tests for it--they makesourcepackage stuff is fairly slow and cumbersome.
<bac> jcsackett: your branch looks more interesting
<jcsackett> bac: it is and it isn't. :-P
<jcsackett> it's only interesting because the template bit has irritating  bits. :-P
<jcsackett> bac: i see at the bottom of your diff the default for has_packages in the method is None; given that it's a boolean switch, shouldn't the default be False, just for readability?
<bac> jcsackett: yeah, probably.  i originally thought about None, True, False but abandoned that idea.  i'll make the change
<jcsackett> bac: cool. thanks.
<jcsackett> bac: r=me. i have requested follow up from sinzui.
<bac> jcsackett: thanks
<jcsackett> one day, i hope, i will not have to append that last sentence when ending reviews. :-)
<sinzui> bac: I approved your branch
<bac> sinzui: i saw that and have sent it off to ec2.  thank you.
<bac> jcsackett: i approved your branch.  you may want to give gavin a chance to look it over, though, out of courtesy.  unless you're super itchy to land it.
<jcsackett> bac: i'm not super itchy, just wanted to get a stamp on it in case gavin is out for a while. if he's back tomorrow i'm going to poke him.
<bac> jcsackett: perfect
<jcsackett> thanks for the review.
#launchpad-reviews 2011-01-04
<wgrant> lifeless: Interested in a very challenging +1/-194 review?
<lifeless> sure
<wgrant> https://code.launchpad.net/~wgrant/launchpad/goodbye-binarypackagereleaseset/+merge/45094
<wgrant> Speaking of reviews, I probably want to find a mentor at some point.
<wgrant> lifeless: Thanks.
<StevenK> wgrant: And a 4 digit bug closure! Win!
<wgrant> StevenK: That was the main purpose of the branch.
<adeuring> gmb: already around?
<gmb> adeuring: Yes. I'll be starting reviews at the top of the hour.
<gmb> Happy new year, by the way :)
<adeuring> gmb: happy new year to you too :)
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || reviewing: adeuring || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> adeuring: I think it's worth refactoring the part of test_user_access_to_private_bug_attachment() that deals with disabling the redirect mechanism in the test HTTP client out into its own method or even a standalone function; it's complex enough that it doesn't really fit in the middle of the test without confusing the reader (well, me, anyway) and we may want to use it elsewhere at some point.
<adeuring> gmb: yes, I'll do that. thanks for the suggestion!
<gmb> adeuring: Cool. I've got a couple of other comments but nothing major, so r=me with that refactoring.
<adeuring> gmb: thanks!
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> happy new year gmb, and thanks for the review!
<gmb> jelmer: Happy new year; you're welcome :)
<mrevell> Hello there gmb. Happy new year and belated congrats on your first anniversary :) I wonder if you'd fancy looking at a branch with two simple text changes? https://code.launchpad.net/~matthew.revell/launchpad/VAT-update/+merge/45113
<gmb> mrevell: Hello, good morning, thankee, thankee and yes of course.
<mrevell> ta :)
<gmb> mrevell: What editor are you using? It appears to be adding DOS line endings (or is that deliberate?)
<gmb> Actually, they're all over the tour HTML files, by the look of it.
<gmb> ... okay, no, I'm wrong. Just tour/api so far...
<mrevell> gmb, Gedit, although the original HTML for the tour was done by an external contractor on some sort of Mac, I believe. AFAIK I'm preserving whatever the file uses at the time. Perhaps I should strip the DOS line endings.
<gmb> mrevell: Yes, I think that would be best. Otherwise, you end up with a situation where we get noise in the diffs just because an editor does something with the line endings.
<gmb> mrevell: Other than that, r=me.
<mrevell> gmb, Thanks. I'll fix those line endings.
<gmb> Cool
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || reviewing: lunch || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* gmb changed the topic of #launchpad-reviews to:  On call: gmb || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> allenap: i updated the mp you were looking at before the break.
<jcsackett> i had bac take a look at it since i wasn't sure how long you were out for, but i'd still love for you to take a look and make sure i addressed your concerns.
<allenap> jcsackett: Okay, cool, I'll go and take a look.
<jcsackett> allenap: thanks!
<bigjools> mrevell: hi, fancy reviewing a UI mockup?
<mrevell> bigjools, I'd love to.
<bigjools> mrevell: https://dev.launchpad.net/LEP/DerivativeDistributions
<bigjools> https://dev.launchpad.net/LEP/DerivativeDistributions#Mockups
<bigjools> in fact
<bigjools> see the first 2
<mrevell> Thanks bigjools, I'll get onto that now.
<bigjools> mrevell: we can have a Grumble if that helps
<bigjools> mrevell: https://dev.launchpad.net/LEP/DerivativeDistributions updated a bit
<mrevell> Thanks bigjools
<Edwin__> gmb: Can you review this branch? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-682727-bug-count-timeout/+merge/45141
<gmb> EdwinGrubbs: Sure.
<gmb> EdwinGrubbs: r=me with a couple of minor nitpicks
* gmb changed the topic of #launchpad-reviews to:  On call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> gmb: thanks
* leonardr changed the topic of #launchpad-reviews to:  On call: leonardr || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to:  On call: leonardr, mars || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* benji changed the topic of #launchpad-reviews to: On call: leonardr, mars || reviewing: - || queue: [benji]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> leonardr and mars: you two can thumb wrestle over https://code.edge.launchpad.net/~benji/launchpad/bug-669701/+merge/45171
<leonardr> i'll get it
<leonardr> mars, then maybe you can review a couple small branches of mine
<leonardr> benji, can you make diff_rules a method of the view instead of a free-floating function?
<benji> leonardr: I can.
<leonardr> can you do anything about that lines_of_context = 99999? is there some 'unlimited' constant?
<benji> leonardr: unfortunately not; it's required to be a number (arithmatic is performed on it)
<leonardr> ok
<benji> re. diff_rules, since it doesn't use self, I assume you want it to be a class method, correct?
<benji> er, not a classmethod a staticmethod
<leonardr> benji: i don't think it matters much, but i don't know much about the best practices of views
<leonardr> maybe mars can weigh in
<leonardr> go ahead and make it a static method. the main thing is that it be close to the place that uses it
<leonardr> i do think your code in line 150-151 violates our style guide
<leonardr> it should be more like
<leonardr> browser.getControl(
<leonardr>  name="field.feature_rules").value = (
<leonardr>   'beta_user some_key 10 another value with spaces')
<leonardr> iow the parenthesis should be the last thing on the line, and you should add parentheses to avoid exceeding the line limit
<benji> ok, I'll change that (that'll be a hard one for me to remember)
<leonardr> same futher along in the diff
<benji> leonardr: hmm, I can't find the second one, what is the line number in the diff?
<leonardr> benji: around 153?
<benji> oh, yeah I already changed that one
<leonardr> ok
<leonardr> i think that's it
<benji> cool, thanks
<benji> leonardr: given that this has a (admin-only) UI component, should it get UI review too?
<leonardr> benji: yes, they may want to change the way you display the diff or something
<benji> leonardr: ok, thanks; do I have to wait until a ui reviewer is on call or is there some other mechanism for getting ui reviews?
<leonardr> benji: you should ping a ui reviewer
<benji> thanks
<benji> sinzui: do you have a moment for a (hopefully quick) UI review?
<sinzui> yes
<benji> sinzui: great! the MP describes how to see the UI change in question: https://code.edge.launchpad.net/~benji/launchpad/bug-669701/+merge/45171
* leonardr changed the topic of #launchpad-reviews to: On call: leonardr, mars || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> mars, my small branches #1:
<leonardr> https://code.launchpad.net/~leonardr/launchpad/launchpadlib-integration/+merge/45174
<mars> leonardr, ok
<leonardr> oops, that's the wrong mp
<leonardr> that's the mp for my launchpadlib branch
<leonardr> 1 sec
<sinzui> benji. When I use other forms, Lp uses an info notification to tell my my change was applied. I can be hacked in the template as you have done, but it is more commonly added using self.request.response.addNotification
<sinzui> Why is this form different
 * sinzui expects to see a div with the class="message info" to make it look like other changes
<benji> sinzui: I used self.request.response.addNotification originally when I just had the "Your changes have been applied" message, but since I had the diff to include addNotification didn't seem appropriate
<benji> and mixing them seemed odd (but I'm glad to do it if that's the status quo)
<benji> the class="message info" seems like a good idea, I'll do that
<leonardr> mars: ok, the branches are
<mars> leonardr, did you push the latest version?  I don't see the "Removed a doctest" bit
<leonardr> https://code.launchpad.net/~leonardr/launchpad/launchpadlib-integration/+merge/45174
<leonardr> and then https://code.launchpad.net/~leonardr/launchpadlib/launchpad-integration/+merge/45176
<sinzui> benji: class="informational message" is correct, but I see you point about oddness.
 * sinzui tries to hack it
<leonardr> mars: i think you're reading the wrong merge proposal? the one i posted to https://code.launchpad.net/~leonardr/launchpad/launchpadlib-integration/+merge/45174 by mistake?
<mars> leonardr, I don't see the "removed a test" bit in the first branch
<leonardr> the doctest removal is in launchpadlib, not launchpad
<mars> leonardr, same message for both commits?
<mars> err, MPs
<leonardr> mars: if you reload https://code.launchpad.net/~leonardr/launchpad/launchpadlib-integration/+merge/45174 you'll see a comment with an updated commit
<leonardr> s/commit/MP/
<mars> ah :)
<sinzui> benji: r=me. I do not want to introduce a blue, yellow, red, and green splotch to a page and risk giving a beloved losa  a seizure
<benji> sinzui: to be clear: you're approving it without any changes?
<sinzui> yes
<benji> cool, thanks
<mars> leonardr, still reading the second one, found a note or two so far
<leonardr> mars, great
<mars> just finished
<leonardr> mars: i have an unrelated question about ec2 test. i'm getting "aws credentials could not be loaded" on my new computer when i try to do ec2 test. do you know what i need to copy over from my old computer?
<mars> leonardr, copy the $HOME/.ec2 directory
<leonardr> thanks
* leonardr changed the topic of #launchpad-reviews to: On call: mars || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* mars changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2011-01-05
<wgrant> StevenK: I can has review?
<StevenK> wgrant: Suppose
<StevenK> wgrant: Can haz link?
<wgrant> StevenK: https://code.launchpad.net/~wgrant/launchpad/bug-694004-log-parser-glob/+merge/45201
<StevenK> thumper: https://code.launchpad.net/~wgrant/launchpad/bug-694004-log-parser-glob/+merge/45201 when you can
<thumper> done
<StevenK> thumper: Ta
<StevenK> wgrant: ^
<wgrant> thumper, StevenK: Thanks.
<wgrant> Julian may unkill me tonight.
<StevenK> If it lands and gets QA'd? :-)
<mrevell> Hello
<flacoste> me
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin, benji || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> Edwin, benji: Could you please review https://code.launchpad.net/~abentley/launchpad/bzr-2.2.2-lp-1/+merge/45258 ?
<benji> abentley: sure, on it now
<abentley> benji, thanks.
* abentley changed the topic of #launchpad-reviews to: On call: Edwin, benji || reviewing: -, abentley || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> abentley: do you want a review of the changes to bzr or just the version bump?
<abentley> benji, just the version bump.  The changes in lp:bzr/2.2 were reviewed by the Bazaar team.
<benji> abentley: ok, in that case: the change looks fine, other than running the tests myself (which I assume you'll do so via ec2 land or have already done) I don't see anything more I can do there; also, if I understand https://dev.launchpad.net/PolicyAndProcess/OptionalReviews correctly, you could have self-reviewed the change
<abentley> benji, I don't believe in optional reviews.  In this case, you could have balked and insisted I write up a test plan, for example.
<abentley> benji, or you could have asked me to include only the fixes I mentioned, rather than using the tip of lp:bzr/2.2
<benji> perhaps I should have reviewed the bzr change, but as-is I'm assuming that the bzr guys' review would have cought anything that I would; other than that I can't see any reason not to approve
<benji> if the tests pass, I'd rather track 2.2 than diverge further from it because I have reason to believe that the bzr guys do a reasonable job in choosing what changes to make there; I assume we don't have a policy regarding that so I feel free following my personal guidelines in that respect
<benji> I'll try to distill this conversation a little and put that in an approval message; shortly after that Edwin-afk can be summoned from the beyond to review my review
<benji> Edwin-afk: when you have a chance, I need a review of my review of https://code.launchpad.net/~abentley/launchpad/bzr-2.2.2-lp-1/+merge/45258
<EdwinGrubbs> benji: I'm on it
<benji> thanks
<EdwinGrubbs> abentley: is the NotBranchError.__repr__() a temporary hack? Will its superclass' __repr__() be simplified also? I'm worried that there isn't a comment to prevent warn someone to not remove the __repr__() that looks redundant.
<abentley> EdwinGrubbs, I didn't write this, but it was intended to be a minimal fix because 2.2 is a stable branch.  In fact, they were not expecting any further changes to it.
<abentley> EdwinGrubbs, mbp's comment in https://code.launchpad.net/~spiv/bzr/just-add-repr-687653-2.2/+merge/43319/ is that there are better fixes, like deleting the code (that does the fancy formatting).
<EdwinGrubbs> abentley: ok, that should be fine. I approved it already. It's fine that a better fix is down the road.
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin, benji || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<benji> leonardr: I made two tiny changes to the MP you approved yesterday (https://code.edge.launchpad.net/~benji/launchpad/bug-669701/+merge/45171) and would like to at least give you a chance to object; the incremental diff is at http://pastebin.ubuntu.com/550813/
<benji> leonardr: I want to make a couple more small changes, but I'm going to get them reviewed by Edwin so you're off the hook
<leonardr> benji: ok, cool
<leonardr> i can take a look at the small changes if you want
<benji> EdwinGrubbs: I have a couple of small changes to a branch that was reviewed yesterday (see my last comment on https://code.edge.launchpad.net/~benji/launchpad/bug-669701/+merge/45171), do you have time to take a look?  The change is pretty small (http://pastebin.ubuntu.com/550823/) but lets us fix two additional bugs with the one branch.
<EdwinGrubbs> benji: I'll look at it in a few minutes.
<benji> great, thanks
<EdwinGrubbs> benji: why is the name of the logger shown in the browser when submitting changes?
<benji> EdwinGrubbs: Martin requested it here: https://bugs.launchpad.net/launchpad/+bug/670019/comments/4
<_mup_> Bug #670019: audit facility for feature flags <feature-flags> <lp-foundations> <Launchpad itself:Triaged by benji> < https://launchpad.net/bugs/670019 >
<EdwinGrubbs> benji: review sent. Just one minor comment.
<benji> thanks
* benji changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2011-01-06
* allenap changed the topic of #launchpad-reviews to: On call: Edwin, allenap || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> jcsackett: Do you fancy a review? 400 lines, nothing very controversial. https://code.launchpad.net/~allenap/launchpad/sub-search-ui-bug-656823-3/+merge/45349
<jcsackett> allenap: sure, i can take a look at that. is it time sensitive? still eating breakfast. :-)
<jcsackett> allenap: looks like all your activereviews already have comments--safe bet someone has already gotten to the one you needed?
<jcsackett> nm; i see those are both screenshots from you. which MP did you need?
<allenap> jcsackett: No rush, https://code.launchpad.net/~allenap/launchpad/sub-search-ui-bug-656823-3/+merge/45349
<allenap> Thank you!
<gmb> allenap: I have a small JS branch for you if you're initerested.
<gmb> Or indeed interested.
<allenap> gmb: I'm doing one of your js branches already.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: gmb || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> allenap: Ah, cool. Since I've only got one in the queue, it's likely to be the same one. :)
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> gmb: And I just finished it :)
<gmb> Hurrah for small units of work.
<gmb> allenap: Ta for the review. I've made the necessary changes.
<allenap> gmb: Cool.
<leonardr> allenap, I have a branch to fix the recent buildbot failures
<leonardr> https://code.launchpad.net/~leonardr/launchpadlib/correct-test-failure/+merge/45370
<leonardr> failure; https://lpbuildbot.canonical.com/builders/lucid_lp/builds/496/steps/shell_6/logs/summary
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: -,- || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> jcsackett: -^
<jcsackett> leonardr: i've looked over it and it looks good. i'm requesting follow up from sinzui, since i'm being mentored.
<leonardr> ok
<leonardr> jcsackett: there's also going to be a launchpad branch which just bumps up the launchpadlib used from 1.9.0 to 1.9.1
<jcsackett> i'll be happy to look at that whenever, since that will be an easy one. it may be a candidate for self review, however (not sure if we formally started doing that).
<jcsackett> leonardr ^
<stub> leonardr: I need the testsuite fix I landed today used by Launchpad soon.
<stub> (launchpadlib testsuite that is)
<leonardr> stub: ok, did you already do this work?
<stub> leonardr: Yes, landed today. One line change to a doctest.
<leonardr> ah, that is a different one line change to a doctest
<leonardr> well, it'll go in to 1.9.1 as well
<stub> Cool. Once launchpadlib tests stop assuming the Librarian is running on port 58000 the sooner I can land this Librarian branch.
<leonardr> jcsackett: here's the launchpad branch: https://code.launchpad.net/~leonardr/launchpadlib/launchpad-integration/+merge/45381
<jcsackett> leonardr: r=me. request for sinzui to follow up already made.
<leonardr> thanks
<jcsackett> np. i like those one line reviews. :-P
<gary_poster> sinzui: ping.  I'm trying to speed up leonardr's testfix review, so I'd like your permission to approve jcsackett's reviews
<sinzui> go ahead
<gary_poster> thank you
<gary_poster> approved
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: -,allenap || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jcsackett changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: -, - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> allenap: some questions and comments on your MP.
<allenap> jcsackett: Cheers, thanks.
<bac> hi allenap, could you do a bugs review for me?
<allenap> bac: Sure.
<bac> https://code.launchpad.net/~bac/launchpad/bug-5927/+merge/45403
<bac> thanks allenap
* allenap changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: bac, - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> salgado, do you want to do a UI review of https://code.launchpad.net/~allenap/launchpad/sub-search-ui-bug-656823-3/+merge/45349
<salgado> sinzui, sure!
<bac> thanks allenap
<allenap> bac: Welcome :)
* allenap changed the topic of #launchpad-reviews to: On call: allenap, jcsackett || reviewing: -, - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<allenap> abentley: Can you have a look at https://code.launchpad.net/~leonardr/launchpad/publish-tokens-2/+merge/34123. I don't know if it's still relevant, but it might warrant an updated vote from you.
<leonardr> abentley, allenap: that code is landed, though possibly not in that branch
<allenap> leonardr: Awesome, okay.
<abentley> leonardr, ACK.
<leonardr> abentley, allena: actually, what happened was we decided to take an alternate approach
<abentley> leonardr, please mark the proposal rejected, then.
<leonardr> i'll mark the branch as abandoned
<salgado> allenap, I'm reviewing the UI of your subscription filter branch and am wondering what they're for...
<allenap> salgado: Hehe :)
<allenap> salgado: They are a new feature, so you can subscribe to a subset of bug mail.
<allenap> salgado: So you could subscribe to only mail for a particular tag, or set of statuses.
<salgado> allenap, ok, and when you want to subscribe to everything then there are no filters -- just the usual structural subscription?
<allenap> salgado: Yes, exactly like that.
<salgado> allenap, but as soon as you add a filter, you only get what matches that filter?
<allenap> salgado: Yes, and if there are multiple filters for a subscription they are ORed together.
<allenap> salgado: So you could have a filter for Fix Committed bugjam2010 bugs, another for open Critical bugs, and you'd get mail for both.
<salgado> right
<salgado> allenap, I'm wondering if it'd make sense to add a sentence to each subscription with filters stating that the user will only get email for bugs matching at least one of the filters?  that was not clear to me at first
<allenap> salgado: Okay, that makes sense. I can move the repeating "Bug mail filter" bit up and add an explanation.
<salgado> allenap, or maybe it could be at the top of the page so that we don't repeat it when there are multiple subscriptions with filters?
<allenap> salgado: Okay, that's cool too. Do you know of an example I can crib?
<salgado> allenap, you mean just the text?  I'll think of something
<allenap> salgado: Yeah, thanks.
<salgado> allenap, are structural subscriptions used for anything other than bug mail?
<allenap> salgado: Blueprints, in theory.
<salgado> allenap, hmm, then I think it'd be weird to talk about bugmail at the top when the subscriptions are not just for bugs.  /me thinks of something else
<allenap> salgado: I have an idea... I'll get a screen-grab for you.
<salgado> allenap, great!
<allenap> salgado: http://people.canonical.com/~gavin/ui/sub-search-ui-bug-656823-3/+structural-subscriptions,%20v2.png
<salgado> I like that
<allenap> salgado: \o/ Cool.
<salgado> allenap, do you think it's worth to mention before the filters that messages that match any of them will go through?
<salgado> to make it clear that it's not just messages that match all of them that go through
<allenap> salgado: Mmm. I'm on the fence about that. I'll scratch my head for a few minutes. Maybe there's a clearer way to denote that.
<salgado> I'm not very fond of that either, but I don't think it's 100% clear that the filters are ORed together
<allenap> salgado: I have to go now, but I'll try to come up with something later.
<allenap> Thank you :)
* allenap changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> allenap, np. I'll suggest something on the MP and maybe sinzui will have other ideas as well
* henninge changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: - || queue: [henninge]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jcsackett: Hi! ;)
<jcsackett> henninge, hello!
<jcsackett> i'm going to guess i know what you need reviewed. :-)
<henninge> No need to guess ...
<henninge> https://code.launchpad.net/~henninge/launchpad/db-devel-697845-import-failure/+merge/45422
<henninge> :)
<jcsackett> henninge, it's sort of frightening when an MP begins "Boy, how do I explain this..." :-P
<henninge> jcsackett: well, with message sharing and upstream-ubuntu sharing, translations is acquiring soyuz qualities when it comes to domain knowledge.
<henninge> Some will argue it has always been that way ...
<jcsackett> lol.
<jcsackett> henninge: so if i understand your notes, the new approach here aims to be more inclusive, rather than the very restrictive approach before?
<henninge> exactly
<henninge> jcsackett: old, basic message sharing shared translations between all series within a distribution source package or between all series in a proudct.
<henninge> jcsackett: the new approach links those two sides using the packaging links.
<jcsackett> henninge: okay.
<henninge> jcsackett: "new" is "recife"
<jcsackett> henninge: right.
<henninge> jcsackett: but it must retain the old notion of "all series within a distribution are sharing translations"
<henninge> and same for product
<henninge> jcsackett: but the first implementation of this did not do that but would exclude a distroseries if its sourcepackage was linked to a different product than the others series
<henninge> but we have those situations in our database
 * jcsackett nods.
<jcsackett> i can follow that.
<henninge> jcsackett: so, as you stated correctly, this approach now looks at all series of distribution source package and the products that the packaging links point to.
<henninge> looking from the other side its "all series of a product and the source packages they are linked to".
<henninge> this is the old sharing behavior *plus* templates from the "other side", not a completely new behavior.
<henninge> jcsackett: EOT ;)
<jcsackett> :-)
<abentley> jcsackett, Could I interest you in reviewing https://code.launchpad.net/~abentley/launchpad/stale-targets/+merge/45418 ?
<jcsackett> abentley, i would be happy to, right after i finish henninge's.
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: henninge || queue: [abentley]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> jcsackett, tx
<jcsackett> henninge, i gather the somewhat frightening subquery in _queryBySourcePackageName is the searching via packaging links?
<henninge> yes ;)
<jcsackett> cool. as big as that is, it's still not terrible to understand.
<henninge> it's the same in _queryByProduct, only that the subquery has to return a tuple.
<henninge> distribution source pacakges are represented by the tuple (distribution, sourcepackagename)
<henninge> "all packages with that name in any series of this distribution"
<jcsackett> henninge: i'm verifying all tests pass, but as far as read through of the code, this looks good. as soon as tests pass i will mark approved and get sinzui to follow up.
<jcsackett> thanks very much.
<henninge> jcsackett: Great, thank! ;) I have to leave now but will come back later to land it.
<henninge> s
<jcsackett> sounds good. :-)
<henninge> good bye!
* jcsackett changed the topic of #launchpad-reviews to: On call: jcsackett || reviewing: abentley || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jcsackett> abentley: is there anything test wise to make certain the widget is showing the things it should not just showing the things it shouldn't?
<jcsackett> i could see someone breaking the widget so it shows nothing and the test still passing, because its only affirming that something isn't there.
<abentley> jcsackett, Yes, there are pagetests that cover the existing functionality.
<jcsackett> abentley: ah, okay.
<jcsackett> abentley: r=me. i have requested follow up from sinzui.
<abentley> jcsackett, cool, thanks.
<jcsackett> is there anyone who could review something for me? https://code.launchpad.net/~jcsackett/launchpad/merge-ppas-697685/+merge/45450
<bac> hi lifeless, does this look about right (re: bug 5927) ?  http://pastebin.ubuntu.com/551305/
<_mup_> Bug #5927: assignee cannot see private bug <disclosure> <lp-bugs> <Launchpad itself:Triaged by bac> < https://launchpad.net/bugs/5927 >
#launchpad-reviews 2011-01-07
<wgrant> Could someone please review https://code.launchpad.net/~wgrant/launchpad/bug-698349-cStringIO-unicode-funsies/+merge/45470?
<wgrant> It is breaking the production upload processor frequently.
<wgrant> mwhudson: Is there a right fix?
<mwhudson> wgrant: i don't know
<mwhudson> wgrant: "not using cStringIO" ?
<wgrant> mwhudson: Possibly. But then you end up writing a unicode to a file, which is not broken, but still wrong.
<mwhudson> oh right
<mwhudson> maybe it is the right fix then :-)
<wgrant> Now to coerce somebody into cowboying it...
<thumper> https://code.launchpad.net/~thumper/launchpad/fix-recipe-build-oops/+merge/45319 anyone?
<StevenK> lifeless: Would you mind mentoring my review of thumper's branch?
<thumper> https://code.launchpad.net/~thumper/launchpad/fix-recipe-build-title/+merge/45474 is dependent of the first
<thumper> StevenK: I suppose it would be bad for me to mentor your review of my change :)
<thumper> StevenK: mumble?
<StevenK> thumper: Sure, give me a few minutes?
<thumper> StevenK: sure
<StevenK> thumper: Ready when you are
<thumper> StevenK: ack, let me just chat with kids briefly
<StevenK> win 40
<StevenK> Oops
<thumper> mwhudson: feel like mentoring StevenK's reviews?  I would but they are for my work
<mwhudson> thumper: link me up
<thumper> mwhudson: https://code.launchpad.net/~thumper/launchpad/fix-recipe-build-oops/+merge/45319
<thumper> mwhudson: https://code.launchpad.net/~thumper/launchpad/fix-recipe-build-title/+merge/45474
<thumper> mwhudson: and thanks
<mwhudson> dammit my brain isn't working
 * mwhudson attempts to get it going
<mwhudson> too damn humid
<StevenK> It's 25degC and threatning to rain here ...
<mwhudson> well yeah, but you're in sydney
<mwhudson> apparently it's 19.4 here and 77% humidity
<mwhudson> feels like both are higher though
<mwhudson> thumper: erm
<mwhudson> thumper: doesn't https://code.launchpad.net/~thumper/launchpad/fix-recipe-build-oops/+merge/45319 mean changing the url of every build ever?
<thumper> yep
<thumper> mwhudson: since the opaque id is still opaque, just different, it seems fine
<mwhudson> is that not going to cause some grief?
<thumper> it shouldn't
<mwhudson> e.g. mail archives
<thumper> wgrant seems ok with it :)
<thumper> ah...
<mwhudson> i guess people don't bookmark builds
<thumper> maybe...
<thumper> historical builds aren't that interesting
<mwhudson> thumper: i also don't understand why lib/lp/code/browser/tests/test_sourcepackagerecipe.py has changed, but that's probably just me being thick
<wgrant> It shouldn't be too much of a problem. Old builds aren't too relevant for long.
<wgrant> It would be nice to avoid breaking everything, though I don't see how that's possible.
<thumper> mwhudson: the breadcrumbs changed
<thumper> mwhudson: as the build is now under the archive
<mwhudson> well, i guess you could have the new urls all be $archive/+newtoken/+newid
<mwhudson> thumper: ah ok
<mwhudson> but +build is by far the sanest value for +newtoken
<thumper> mwhudson: I wanted the builds to be similar
<thumper> that's why I piggy backed on the binary package build url
<mwhudson> yeah, i can see that
<mwhudson> i think it's probably fine
<wgrant> We should probably clear it with bigjools first. But it's unlikely that many old URLs are going to be used much.
<thumper> wgrant: ok
<thumper> mwhudson: feel free to punt to bigjools
<thumper> mwhudson: and go have a beer
<thumper> it is almost time :)
<mwhudson> yeah indeed
<mwhudson> i want to make a branch to land the twisted fix that just landed though
<thumper> which fix is that?
<mwhudson> thumper: http://twistedmatrix.com/trac/ticket/4395 and so https://bugs.launchpad.net/bzr/+bug/556132
<_mup_> Bug #556132: bzr: ERROR: paramiko.SSHException:  Key-exchange timed out; consistent after sending 1GB data <lp-code> <paramiko> <Bazaar:Confirmed> <Launchpad itself:Triaged> <Twisted:New> < https://launchpad.net/bugs/556132 >
<thumper> w00t
<mwhudson> grr
<mwhudson> https://code.launchpad.net/~mwhudson/launchpad/include-twisted-4395-fix/+merge/45483 <- review anyone?
<mwhudson> thumper: still by the keyboard?
<mwhudson> oh i guess i could self review it if i liked
<lifeless> StevenK: still need a hand?
<StevenK> lifeless: Nope, mwhudson stood in, thanks
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> hi adeuring
<adeuring> hi bac
<bac> busy day?
<adeuring> bac:  just a heads-up: I'm currently looking into sutb's/sinzui's MP
<bac> ok
<bac> adeuring: you should push that 'claim' button!  :)
<adeuring> bac: yeah, I'll do it ;)
<allenap> salgado: Hi, do you have time today to have a look at the updates I've made to https://code.launchpad.net/~allenap/launchpad/sub-search-ui-bug-656823-3/+merge/45349?
<salgado> allenap, wow, that looks really nice
<allenap> Oh brilliant :)
<salgado> allenap, is the +structural-subscriptions page protected or is it zope.Public?
<allenap> salgado: launchpad.View, but anyone can view.
<allenap> iirc
<salgado> allenap, ok. I asked because if it was restricted to the user himself, we'd be able to avoid repeating the user's name in the sentence about filters
<allenap> salgado: Yeah, originally I wrote "you" in several places before I realised that anyone could view it.
<allenap> sinzui: You've already reviewed the code for https://code.launchpad.net/~allenap/launchpad/sub-search-ui-bug-656823-3/+merge/45349, so would you also have time to mentor salgado's UI review?
<sinzui> okay
<allenap> Thank you.
<sinzui> allenap: you are going to hate me
<allenap> sinzui: You *hate* it?
<sinzui> allenap: There is a growing movement, once certainly in design/UX that italic is should not be used in interfaces
<sinzui> they prefer bold, which you are already uses
<sinzui> allenap: I broken this rule with the registrant line in the header
 * sinzui is bad
<allenap> sinzui: If that's all then I most certainly don't mind at all :) I'll replace the italics with bold.
 * allenap suspects that's not all.
<sinzui> allenap: I think this is good to land. I believe we will revisit all uses of italics in Lps CSS this year
<sinzui> oh
<allenap> sinzui: Cool. So I should leave the italics?
<sinzui> allenap: why are the rules in side bar portlets
<sinzui> what other pages uses them in the main content
<allenap> sinzui: I don't know what you're talking about :)
<sinzui> eg, shaded boxes with round corners. We just removed a few in December because they were odd
<allenap> sinzui: Ah, they're not side-bar portlets, I just added did a diff with rounded corners to group the text with the product link, which is a bit small as a delimiter on its own.
<sinzui> I think we should use hr or just portlet class to create a top border
<allenap> s/diff/div/
<allenap> sinzui: ISTR that hr has been neutered with CSS, so portlet class instead I guess, but that seems a bit semantically wrong.
<sinzui> allenap: I think we are discovering a common problem here
<allenap> Yeah.
<sinzui> https://launchpad.net/prohttps://launchpad.net/projects/+index?text=gedit
<sinzui> In our picker, we use lines between rows and shade. That is certainly the approves way, but I do not think we every uses that outside of the picker
<sinzui> allenap: I think we should treat this as something separate from your branch. This change is only for users participating in the beta?
<allenap> sinzui: The page is not restricted at all, by flags or otherwise, but it is not navigable other than by manipulating the URL by hand.
<sinzui> fab
<sinzui> You ca try portlet for now, but I will report a bug that describes the general problem with some hope that we will fix several pages when we know what is right.
<sinzui> I approve your branch with the use of bold and the removal of the background shading
<allenap> sinzui: Okay, thanks!
<allenap> sinzui: Would you accept it with a #ddd border (like bug comments), with or without rounded corners?
<sinzui> yes
<allenap> sinzui: Fwiw, like http://people.canonical.com/~gavin/ui/sub-search-ui-bug-656823-3/+structural-subscriptions,%20v5.png
<sinzui> the rounded corners look od
<sinzui> interesting style though
<allenap> sinzui: I'm fond of them but I don't mind dropping them.
<sinzui> Do we use that style else where?
<allenap> sinzui: I can't think that we do.
<sinzui> allenap: They are reminiscent to Ubuntu/Canonical designs
<allenap> sinzui: Ah, yes.
<sinzui> I do not want to add exceptions. I think I want all comments to look like that though
<allenap> sinzui: I'll land without rounded corners, and we can address all of these group-boxes with a class later on.
<sinzui> I agree
<allenap> sinzui: Cool, thank you.
<allenap> sinzui: Interesting. Your code vote was changed into a ui vote, rather than registering both.
<sinzui> that sucks
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On call: bac || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> bac: super tiny branch: https://code.launchpad.net/~leonardr/launchpadlib/fix-for-martin/+merge/45540
<bac> ok
<bac> leonardr: done
<leonardr> thanks
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<abentley> bac, could you please review https://code.launchpad.net/~abentley/launchpad/no-queue-rescore/+merge/45560 ?
<bac> abentley: sure
<abentley> bac, also, who's our UI review manatee?
<bac> abentley: salgado and mrevell
<abentley> bac, thanks.
<salgado> abentley, I may not be able to review that as I'm EOD and will be at the platform rally next week
<abentley> salgado, Ah.  Monday would have been good enough, but if you're away, I'll ask mrevell.
<salgado> if he can't do it on Monday, I'll try to find some time for it during the sprint
<abentley> salgado, cool, thanks.
<bac> hi abentley, the meat of your fix looks really good.
<bac> abentley: stylistically, i'm a little concerned by you putting a comment before a method's docstring.
<abentley> bac, you mean you'd rather it went after the docstring?  Before causes lint errors.
<bac> abentley: i always thought docstrings were the first thing...have we done that elsewhere?
<bac> abentley: it just looks funny so i thought i'd see what you thought
<abentley> bac, I'm fine with moving it after the docstring.
<bac> without spending too much time pondering it.
<bac> ok.  thanks for cleaning up the lint
<bac> i know it is a pain
<bac> r=bac
<abentley> bac, thanks.
<abentley> bac, do you know whether we've handled similar error scenarios before?
<bac> abentley: you mean to handle stale views?
<bac> er, stale URLs i guess i should say
<abentley> bac, yeah, situations where you shouldn't be at that view at all, whether viewing or submitting.
<bac> i can't recall seeing it anywhere
<abentley> bac, Cool.
* bac changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
