#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
{'channel': 'launchpad-reviews', 'month': 1}