#launchpad-reviews 2009-10-01 <stub> EdwinGrubbs: https://code.edge.launchpad.net/~stub/launchpad/librarian-gc/+merge/12695 if you are still there. * stub changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <al-maisan> Good morning! * noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com * noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: stub || queue: [] || This channel is logged: http://irclogs.ubuntu.com <noodles775> stub: hi! Just looking at your MP... would it be worth in addition creating a separate condition to log a warning for this situation where the file exists on disk but not in the db? <stub> That is actually normal - client.addFile(foo,...); transaction.abort() leaves a file on disk <noodles775> OK - I assumed that the transaction is only aborted in abnormal circumstances. <stub> Nah - anytime a web request needs to be retried after uploading a file for instance. <noodles775> k. great. * noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com <stub> noodles775: test_deleteUnwantedFiles_bug444 or test_delete_unwanted_files_bug444 ? <stub> noodles775: The former matches the existing test method names more closely, the latter is correct as the function being tested is called delete_unwanted_files() <noodles775> stub: according the the test style guide, I thought the method name should *match* the real method name... <stub> its a function being tested, not a method, and its name is delete_unwanted_files() <stub> I'd probably go for the latter option myself, which I think is correct per the guide, and stuff the existing naming in that file (circa 2005 IIRC) <noodles775> yep, so I'd use the latter too. <stub> k * noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <allenap> noodles775: Fancy a simple branch? https://code.edge.launchpad.net/~allenap/launchpad/more-retest/+merge/12709 <noodles775> allenap: sure thing! <allenap> noodles775: Thanks :) <thumper> deryck: https://code.edge.launchpad.net/~thumper/launchpad/inline-lifecycle-status-edit/+merge/12707 <noodles775> allenap: nice - I hadn't seen takewhile before... <noodles775> allenap: on line 97, why are you converting to a set rather than a list? <noodles775> I mean, test output will not have duplicates...? * noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: allenap || queue: [] || This channel is logged: http://irclogs.ubuntu.com <allenap> noodles775: If multiple test logs are passed in (one of the new features), this removes duplicates. <noodles775> allenap: then why not convert it back to a sorted list then and there? <noodles775> ie., so extract_tests returns a sorted list of unique tests? <noodles775> That's great that multiple logs can be used - and that it's now using a buildout template! <allenap> noodles775: I only want them sorted to display them prettily, otherwise a set is what I want. Not much in it really. <allenap> noodles775: As in, could be either a set or a list, but it happens to be a set because of de-duplication. <noodles775> oh, I didn't see the set features being used anywhere... looking again. <noodles775> Ah, gotcha. ok. <noodles775> allenap: great, r=me... just note, there's one reference to the old utilities/retest in the usage instructions. <noodles775> er, actually, two :) <allenap> noodles775: Ah, good spot, thanks :) * noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <adeuring> noodles775: could you please review a sequel of my last MP: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-refactor-haldevice-2/+merge/12713 ? <noodles775> adeuring: ah great... I get to find out how the new HW character is introduced :) <mrevell> noodles775: I have a branch fixing the problems we discussed: https://code.launchpad.net/~matthew.revell/launchpad/bugs-439909-439267/+merge/12710 <noodles775> mrevell: great, thanks for that! <noodles775> mrevell: doing yours first as it's tiny :), I was just checking to see if there were any tests that might check the url for the contact-us link... <noodles775> mrevell: and there aren't, but: http://pastebin.ubuntu.com/282928/ <noodles775> there's a few other places where we say 'Contact us' but send people to zstrickland@example.net? <noodles775> mrevell: I'll leave it up to you whether you think it's worthwhile making them consistent. <noodles775> mrevell: r=me <mrevell> noodles775: Yeah. The first of those, main-template, I believe is an over hang from the old design. I'll fix the other links. Thanks. <noodles775> Great. <noodles775> mrevell: and, of course, run it through ec2 first as there could be a test somewhere looking for the contact us url/href etc. * noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com <noodles775> mrevell: out of interest, did you find out what's behind the new version.txt file (ie. should it be updated automatically or at least as part of the rollout process?) <mrevell> noodles775: sinzui didn't know where it had come from, but I'm going to ask beuno. I'm assuming it'll need a manual update. <noodles775> hrm... well it was added with r9425 fwiw - check the commit msg there for more details. <noodles775> adeuring: gee, that's an unfortunate diff... <adeuring> noodles775: why? <noodles775> adeuring: Nothing wrong with your code, I jut meant the huge segment of HALDevice that diff has assumed has been deleted and added. <adeuring> noodles775: Ah, OK. <noodles775> adeuring: r=me <adeuring> noodles775: thanks! <noodles775> adeuring: did you try using the revision spec '-r x..y' with bzr send so that the colour-coded diff on the mp is just the revisions you want? If not, might be good for the third installment :) <adeuring> noodles775: I wasn't aware of that option. Thanks for the hint! <noodles775> great, np! <maxb> Is that bzr send -r ---> MP thing documented anywhere? <noodles775> maxb: bzr help send <beuno> maxb, https://help.launchpad.net/Code/ReviewEmail%20interface <noodles775> at least, that documents that the option exists :) <noodles775> ah, even better. <henninge> Hi! I want to use a TAL formatter in a view and I found out I can do it this way: https://pastebin.canonical.com/22831/ <henninge> Is that the right way to do it? Or should I go through the registration somehow? I remember seeing an example once that was different ... <henninge> jtv: ^ * noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com * noodles775 finishes reviewing for the day. * noodles775 changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <barry> noodles775: dang! <barry> is anybody available to review a 279 line diff? <noodles775> barry: there should be some others around soon... :) <barry> noodles775: i guess not cprov-afk though :( * barry waits for rockstar and leonardr * rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <rockstar> barry, leonardr doesn't have a OCR day. <leonardr> but, barry, if you want me to review something specific, i can <barry> leonardr: no? you're listed on https://dev.launchpad.net/ReviewerSchedule <barry> rockstar, leonardr it's xmlrpc and mailman. don't fight over it :) * rockstar yells "Not it!" <rockstar> :) <barry> :-D <rockstar> barry, I'll take it. <barry> rockstar: thanks! sending the mp now * rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: barry || queue: [] || This channel is logged: http://irclogs.ubuntu.com <leonardr> barry: that wiki page is probably from when i was being mentored <barry> leonardr: right. you're not doing ocr anymore? do you want to remove your name from that list? or ic an do it. <leonardr> barry: i'm doing it now <leonardr> (removing my name--i'm not doing ocr, i went through mentoring because it was ridiculous to review lazr.* branches and then have to get someone else to do a 'real' review) <barry> leonardr: sounds good. rockstar sorry for the delay. was otp. finishing up the mp now <rockstar> barry, yeah, I just looked for your proposal and couldn't find it. Was about to bug you. <barry> rockstar: sent <bac> rockstar: can i add a MP to your queue? <rockstar> bac, sure. <bac> rockstar: cool, still waiting for it to show up <rockstar> barry, did you request me as reviewer? <barry> rockstar: oops <barry> rockstar: done now <rockstar> barry, I don't see it on lp reviews either. <barry> rockstar: https://code.edge.launchpad.net/~barry/launchpad/403606-expat/+merge/12734 <bac> rockstar: https://code.edge.launchpad.net/~bac/launchpad/bug-422128/+merge/12735 * bac changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: barry || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com <rockstar> bac, cool, thanks. <rockstar> barry, I really only have one comment, and it's a nitpick really. <bac> rockstar: i need to go get a samwich. you ok with me being awol a bit? <rockstar> barry, you use "Binarys" on line 59 of the diff. I realize Binary is a class name, but it still looks funny. <barry> rockstar: how about Binary's ? (kind of still icky) <rockstar> bac, if you're not here when I get to it, I'll note my questions in the MP and you can address them when you get back. <bac> rockstar: that is a sensible plan of action. see you in a bit. <rockstar> barry, even worse. Bad apostrophe. Maybe "Binary instances" or something. <barry> rockstar: i like that! <rockstar> I mean, it could probably stay as it... <barry> easy to change <rockstar> barry, other than that, it seems sane to me. <barry> rockstar: thanks! * rockstar changed the topic of #launchpad-reviews to: on call: rockstar || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <abentley> rockstar: Could you please review https://code.launchpad.net/~abentley/launchpad/bzr-2.0.0-lp-1/+merge/12738 ? <rockstar> abentley, yessiree <abentley> rockstar: Thanks. <rockstar> abentley, I'm assuming the actual source has landed in download-cache? <abentley> rockstar: Yes. There's not much involved in that. Commit, push, wait. <rockstar> abentley, alright, r=me <abentley> rockstar: Thanks. <abentley> rockstar: Could you please review https://code.launchpad.net/~abentley/launchpad/diffstat-count/+merge/12720 ? <bac> thanks rockstar. <rockstar> bac, you are welcome. <rockstar> abentley, looks good. r=me <abentley> rockstar: Thanks. <abentley> rockstar: Could you please review https://code.launchpad.net/~abentley/launchpad/move-linked-bugs/+merge/12736 ? <rockstar> abentley, it doesn't look like you need the yui-u div around source revisions now. <abentley> rockstar: quite possible. It's all cargo-culted. <rockstar> abentley, yeah, I figured. <rockstar> abentley, so, source-revisions should be wrapped in a yui-g, but that's it. <rockstar> abentley, other than that, r=me. <abentley> rockstar: I'm confused. Change the u to a g? <rockstar> abentley, yea, and remove the first, or that may result in weirdness. <rockstar> abentley, yui-g is a row, yui-u is a column <abentley> rockstar: I miss tables. <abentley> So do this and I can land it? http://pastebin.ubuntu.com/283191/ <rockstar> abentley, yeah. Take a look at what the page looks like, but basically, yes, do that. <abentley> rockstar: Oh great, now I have to push some branches to launchpad.dev <rockstar> abentley, I just use the factory to make fake revisions. <rockstar> Oh wait, that won't work for BMPs though, I don't think. <rockstar> My computer hard locked a bit ago, so if someone was asking for a review, I haven't seen it. Please repost. <rockstar> Okay, so, I have to say this again just in case I missed something when my system locked again. Did anyone request a review from me? <rockstar> Also, desktop effects suck. <leonardr> rockstar, can you do a quick review of https://code.edge.launchpad.net/~leonardr/launchpadlib/release/+merge/12746 ? <rockstar> leonardr, sure. * rockstar changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com #launchpad-reviews 2009-10-02 <al-maisan> Good morning! * adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <jtv> henninge: care to review? <jtv> https://code.edge.launchpad.net/~jtv/launchpad/bug-440445/+merge/12777 <henninge> jtv: so you are assuming that it will also need access to language and translator? <jtv> henninge: yes; it's looking at TranslationGroup to check for membership, and that definitely involves Translator. <henninge> jtv: valid assumption, I guess. <henninge> jtatum: r=me <jtv> Not sure about Language, but it's one of those things that are likely to be fetched by the ORM as a side product <jtv> henninge: no, the other person. :) Thanks. <henninge> jtv: r=me <jtv> :) <henninge> jtatum: whatever branch you want reviewed, I have *not* approved it! ;-) <henninge> jtatum: sorry to bother you ... <henninge> jtv: please remember to set the mp to "approved". For some reason I cannot do it. <jtv> henninge: sure, thanks <al-maisan> hello adeuring -- could you please have a look at a small/simple branch? <al-maisan> it is here: https://code.launchpad.net/~al-maisan/launchpad/cmsg-425800/+merge/12774 <adeuring> al-maisan: I'm already doing this :) <al-maisan> adeuring: ah, wow! <al-maisan> adeuring: I am duly impressed :) <adeuring> al-maisan: there are two new tests: test_requestMirror_doesnt_demote_branch() and test_requestMirror_can_promote_branch(). Perhaps I'm missing something obvoius there -- but isn't future_time in the latter test a timestanp in the past: future_time = datetime.now(pytz.UTC) - timedelta(days=1) ? * al-maisan looks <al-maisan> adeuring: argghh <al-maisan> the diff generated for the MP is wrong <al-maisan> just a minute <al-maisan> adeuring: http://pastebin.com/m4c94727b <al-maisan> sorry about that <al-maisan> this is a *really* simple branch <al-maisan> bbiab <adeuring> al-maisan: Ah, indeed quite much shorter ;) <adeuring> al-maisan: but the other test is there nevertheless ;) <al-maisan> adeuring: aha <al-maisan> adeuring: I don't see any of that in the diff I provided you with * al-maisan rubs his eyes <adeuring> al-maisan: what I menat: I am simply curious about this test, even it is is not part of the patch. <al-maisan> adeuring: yep, 'future_time' would have to lie in the past .. provided the time on the test machine is set correctly :) * adeuring is quite confused... future time in the past? <adeuring> al-maisan: but let's put that aside. <adeuring> al-maisan: al-maisan: just one question about the short diff: You removed the text "Did you mean to upload to a PPA?". Isn't this sometimes a useful hint for users? <al-maisan> adeuring: not really .. it would rather confuse people <adeuring> al-maisan: OK, so r=me <al-maisan> adeuring: thanks <allenap> adeuring: Fancy a devscripts.ec2test branch to review? <allenap> https://code.edge.launchpad.net/~allenap/launchpad/ec2-test-race-bug-422433/+merge/12778 <adeuring> allenap: sure <allenap> adeuring: Thanks :) <bigjools> al-maisan: don't remove that error text <al-maisan> bigjools: alright; what do you suggest instead? <bigjools> al-maisan: I don't have any context - what are you doing? <al-maisan> bigjools: https://bugs.edge.launchpad.net/soyuz/+bug/425800 <mup> Bug #425800: confusing message if signer can upload packages from some package sets but not others <packagesets> <soyuz-upload> <Soyuz:In Progress by al-maisan> <https://launchpad.net/bugs/425800> <bigjools> al-maisan: that error text is still correct, but only if the uploader has no packagesets <bigjools> he can upload to <al-maisan> bigjools: that's not quite right. <al-maisan> this error message indicates that the uploader has no upload permissions whatsoever <bigjools> al-maisan: yes, that's exactly what I mean <bigjools> so the existing check for that error text needs to also check packagesets <al-maisan> however, he may end up in this situation if he has *some* package set based permissions but is lacking one <bigjools> no, that is wrong <al-maisan> hmm .. I looked at the code .. the source package based permissions (including package sets) are checked before this error is raised <bigjools> the error should be raised if the upload has no component-based *and* no packageset-based permissions <al-maisan> is that error (formulated that way) useful in first place? <bigjools> yes - it distinguishes between NO permissions and SOME permissions but the wrong ones <al-maisan> aha <bigjools> PPA packages fall foul of it a lot, hence the extra text * al-maisan takes another look at verify_upload() <bigjools> al-maisan: basically if the uploader is trying to upload to say, main, but only has universe permissions, this error is not used <bigjools> the same needs to apply with packagesets <al-maisan> bigjools: I see what you mean. <bigjools> great <adeuring> allenap: r=me <allenap> adeuring: Thanks :) <adeuring> allenap: what I forgot to say iin the review: thanks for splitting the diff into two parts! <allenap> adeuring: I'm glad it helped :) <allenap> adeuring: I replied to your review of ec2-test-race-bug-422433 with an incremental diff. Could you sanity check it, then I'll send it all off to ec2? Thanks. * barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com <adeuring> allenap: looks good. Thanks for the change! <allenap> adeuring: Thanks. <noodles775> barry, have you checked some bug pages also? The "Bug #440220 reported by Barry Warsaw 16 hours ago" is also using the registration slot iirc... <mup> Bug #440220: registering slot interferes with watermark <post-ui-3-cleanup> <Launchpad Registry:In Progress by barry> <https://launchpad.net/bugs/440220> <noodles775> which means that it will *always* wrap (unless you've got a 2m screen ;) ). <barry> noodles775: right. let me paste a bug page s/s <barry> noodles775: attached two more s/s. they do always wrap but it doesn't look too bad to me <noodles775> barry, I think the thing I don't like is that it's wrapping when it doesn't need to. <noodles775> Have you tried leaving it where it was after adding the no_wrap on the app-tabs? <barry> noodles775: it will run into the tabs if the registering contents are long (long person name). maybe that's rare enough to not worry about it though? <noodles775> barry, if you give the registration slot a large left-margin (maybe 2em) it shouldn't get too close? (and will wrap since the app tabs can't, iirc?) <barry> noodles775: e.g. this does not look as good to me: http://launchpadlibrarian.net/32862698/ff-nowrap.png <barry> noodles775: that's a possibility too. i kind of like that the registering information sits neatly above the portlet <noodles775> barry, hmm.... to me it often (ie. with longer slots like on bugs pages) looks like it's been squashed into a space that's too small. <noodles775> barry, I personally agree with rockstar's comment - if it was just below the app tabs, it wouldn't need to wrap at all, and would not interfere... but maybe there's a reason for not doing that? <barry> noodles775: it is a pretty small space (21%). <barry> noodles775: i definitely want to explore that, but iirc beuno really wants that registering slot on the right side <barry> noodles775: so i kind of wanted to run that to ground to see if we could make it work <noodles775> barry, it would be on the right side wouldn't it? I just mean a little bit down from where it is in the last link above. <barry> noodles775: hmm... possibly so. give me a few minutes and i'll try that <noodles775> k <barry> btw, thanks! <noodles775> np! <barry> noodles775: take a look at the s/s in comments 13-16 <noodles775> barry, what do you think? My first thought is that the only thing that looks ugly about it is the super long username? <barry> noodles775: agreed. i just did that to see worst case. this works for me <noodles775> barry, great. Thanks for fixing it! <barry> noodles775: thanks! can i request you as one of the two ui reviewers for the branch? <noodles775> barry, sure! <barry> awesome <barry> adeuring: ping <adeuring> barry: yes? <barry> adeuring: have time for a 39 line CSS review? <adeuring> barry: sure <barry> cool. sending mp now <adeuring> barry: I have also an MP (a bit larger than yours, but just trivial refactoring): https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-refactor-haldevice-3/+merge/12733 could you review it? <barry> adeuring: sure! this feels so incestuous though :) <adeuring> barry: ;) <adeuring> barry: r=me <barry> adeuring: thanks! * barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: -,adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com <barry> adeuring: did you request me as a reviewer on your branch? <adeuring> barry: no <barry> adeuring: url? <adeuring> barry: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-refactor-haldevice-3/+merge/12733 <al-maisan> hello adeuring, can you please have a look at the fix for bug #425800 (2nd try) ? <mup> Bug #425800: confusing message if signer can upload packages from some package sets but not others <packagesets> <soyuz-upload> <Soyuz:In Progress by al-maisan> <https://launchpad.net/bugs/425800> <al-maisan> https://code.launchpad.net/~al-maisan/launchpad/cmsg-2-425800/+merge/12791 <adeuring> al-maisan: sure <al-maisan> adeuring: thanking you :) <barry> adeuring: when you raise NotImplementedError in your properties, you don't need to instantiate them if they have no arguments. Python will DTRT for you here, and it's also somewhat more efficient <adeuring> barry: Ah, thanks! didn't know that <adeuring> (Though I hope the excpetion will never be raised ;) <barry> adeuring: if the exception has arguments, instantiating it in place is the right thing to do though. <barry> adeuring: right! :) <adeuring> al-maisan: r=me <al-maisan> adeuring: thank you very much! <adeuring> al-maisan: immer gerne :) <al-maisan> adeuring: :) <barry> adeuring: with that fix, r=me <adeuring> barry: thanks! * barry changed the topic of #launchpad-reviews to: on call: adeuring, barry || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com <barry> rockstar: ping <rockstar> barry, yo <barry> rockstar: hiya! could you do a quick second ui review on https://code.edge.launchpad.net/~barry/launchpad/440220-slot/+merge/12786 <rockstar> barry, sure. <barry> rockstar: thanks! <mrevell> barry: Can you take a look at small help branch? https://code.edge.launchpad.net/~matthew.revell/launchpad/bug-215798/+merge/12792 <barry> mrevell: sure thing <mrevell> thanks barry <barry> mrevell: did you forget to bzr add register-branch.html perhaps? it's not in the diff * mrevell hangs head in shame <mrevell> barry: Yup. Sorry. Pushing new version now <mrevell> barry: done <barry> mrevell: can you add the file as a comment. i don't think the mp will update the diff <rockstar> mrevell, I have bzr ci aliased to `bzr commit --strict` and I use that all the time. It makes sure you don't have any rogue files laying around. <barry> rockstar: +1 <rockstar> barry, it will update the diff. <mrevell> barry: I thought we had moving diffs now <mrevell> rockstar: thanks for the top! <rockstar> barry, it updates the diff on push of the source branch now. <barry> rockstar: \o/ <mrevell> s/top/tip <rockstar> barry, abentley is pretty amazazing. <barry> rockstar: he's amazing, too! <mrevell> :) <rockstar> barry, I was using a word from a movie, so the apparent typo was actually purposeful. <barry> rockstar: oh! what movie? <rockstar> barry, it's from "The Benchwarmers" The dialogue goes something like "You guys think you're ath-a-letes now?" "I didn't know athletes had three syllables. That's amazazing!" <barry> mrevell: r=me <mrevell> thanks barry <barry> rockstar: guess i have to put that on my netflix now :) <rockstar> barry, it's appropriate for Max too. <barry> rockstar: great review quotes: "filled with sprays of vomit, fountains of spit and enough hot body air to launch a flotilla of passenger balloons" and "a movie packed with so many idiot characters that rob schneider is cast as the cool guy -- and sort of pulls it off" * barry clicks add to queue :) <rockstar> barry, :) * adeuring changed the topic of #launchpad-reviews to: on call: barry || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <barry> EdwinGrubbs, rockstar ping <EdwinGrubbs> barry: pong <barry> hi EdwinGrubbs can you take a quick look at a testfix diff for me? <barry> EdwinGrubbs: http://pastebin.ubuntu.com/284096/ <EdwinGrubbs> barry: is the facetmenu class have any stylesheets, or is it just a marker? <barry> EdwinGrubbs: it has some css, but base-layout.txt doesn't test it. it is a horrible horrible test, so this just repairs it in the easiest possible way <EdwinGrubbs> barry: r=me <barry> EdwinGrubbs: thanks! testfix here i come #launchpad-reviews 2009-10-05 <al-maisan> Good morning! * intellec` changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com * intellec` changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: jtv || queue: [gmb] || This channel is logged: http://irclogs.ubuntu.com * intellec` changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com <intellec`> gmb: i wonder if a name like forceUpdateAll or something like that would be more appropriate for the method you've added. it's a bit confusing to read that there are no watches to update, and yet when you call updateAllWatches some update <gmb> intellectronica: Hmm. Yes, I see your point. forceUpdateAll() also lends the method an air of danger, which it posesses in spades :) <gmb> intellectronica: I'll change the method name. <intellectronica> cool <intellectronica> gmb: r=me with that change. nice branch! <gmb> intellectronica: Thanks :) <BjornT> gmb: i have a few questions about that review <BjornT> gmb: what happens if you 1) run checkwatches.py --all 2) Ctrl-C:s since it's taking too long 3) restart checkwatches.py without the --all option? <gmb> BjornT: If you run checkwatches.py --all without specifying a bugtracker it'll be ignored, for a start. If you kill it in the middle of a run then the watches will be left with lastchecked=None, because checkwatches deliberately just re-raises KeyboardInterrupt and SystemExit exceptions. <BjornT> gmb: right, that's what i thought (regarding bug watches being left with lastchecked=None) <gmb> BjornT: So I guess what we could do is have Ctrl-C mark the watches as lastchecked=datetime.now(), which would stop them from being updated next time. That seems a bit kludgy though. Alternatively we could keep a mapping of watch id => lastchecked time and just restore the old data. <gmb> Or, hey, we could roll back :) * gmb has just checked where the transaction boundaries lie. <gmb> Hmm. <BjornT> gmb: it's nothing i would expect from using --all (and no, you can't roll back, since you commit the transaction after resetting) <gmb> BjornT: No, you right, we can't. Thought it was in the try: except block. <gmb> BjornT: So, the simple solution is setting lastchecked=datetime.now() for all watches. <BjornT> gmb: i think the behaviour is fine, just unexpected. i think renaming --all to --reset would make things clearer. then it's more explicit <gmb> BjornT: Okay, I agree with you. <gmb> I'll make that change now. Thanks. <BjornT> gmb: thanks. also, don't we run checkwatches.py in some test already? <gmb> BjornT: Yes, we do, but I wanted to demonstrate that this could be done from the command line. <BjornT> gmb: you shouldn't do that by starting a sub process. the LaunchpadScript class allows you to pass in arguments to it for testing <gmb> BjornT: Oh, okay. Didn't know that. I'll update the test to use that, then. <BjornT> gmb: that way you can make the test more useful, instead of checking that passing --all has the same effect as not passing --all ;) <gmb> :) <gmb> Righto. <gmb> BjornT: Hang on, though, how do I import from cronscripts/checkwatches? I thought cronscripts wasn't in the PYTHONPATH. <BjornT> gmb: you don't. you move the CheckWatches class to somewhere you can import it. <gmb> BjornT: Ah, right. Figured you might say that :) <matsubara> ow <matsubara> oops * abentley changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: gmb, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <abentley> intellectronica: Could you please review https://code.launchpad.net/~abentley/launchpad/suppress-resubmit/+merge/12872 ? <intellectronica> abentley: sure <abentley> intellectronica: Thanks. <intellectronica> abentley: so what's the story with those lint warnings? <abentley> intellectronica: Dunno. Bogus lint warnings like that have been showing up for months. <intellectronica> abentley: in wonder if it would be better to move the check into findRelatedBMP <intellectronica> fair enough <abentley> intellectronica: Well, it would be findRelatedAndNotSupersededBMP then. <intellectronica> no, i meant to also parameterize it <intellectronica> but now i see that it's not in the interface. if it's just an internal function then i guess it doesn't really matter much <abentley> intellectronica: I'd be happy to change it to something like findRelatedBMP(include_superseded=False) <intellectronica> abentley: yes, that's what i meant. your call <abentley> intellectronica: I've pushed a version that does that. Here's the diff: http://pastebin.ubuntu.com/286266/ <intellectronica> abentley: great. r=me <abentley> intellectronica: Thanks. <allenap> abentley: Can you review a very small trivial wording change for me? https://code.edge.launchpad.net/~allenap/launchpad/just-comment-on-question-bug-114710-wording-change/+merge/12879 <abentley> allenap: sure <abentley> allenap: r=me. <allenap> abentley: Thanks. * sinzui1 changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: gmb, - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com <intellectronica> sinzui1: what do you need reviewed <intellectronica> ? <sinzui1> intellectronica: I do <sinzui1> https://code.edge.launchpad.net/~sinzui/launchpad/delete-structural-target/+merge/12877 <intellectronica> oright, i'm reviewing it * intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: sinzui, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <intellectronica> sinzui1: are the changes to launchpad-login.pt and style-3-0.css really part of what you want me to review? <intellectronica> sinzui1: also, isn't destorySelf an sqlobjectism? <intellectronica> sinzui1: also, does it really make sense to disallow the deletion of a milestone with subscribers? anyone can subscribe to the milestone, so this can make it very inconvenient for the project team. i'd expect to be able to delete it with the subscriptions deleted too <intellectronica> sinzui1: ah, i think i understand what's going on. you're doing that, but in the view. the problem is that the behaviour is then different if you interact with the model directly (through the API, for example) <intellectronica> sinzui1: also nm my first question. i now see why you did want me to review those changes <intellectronica> sinzui1: so the code looks fine, but i really think the work should be done in the model and not in the view. what do you think? <sinzui1> intellectronica: yes, the login style fix is required because .subordinate must work as beuno wants <intellectronica> yeah got it once i read the rest of the diff <sinzui1> intellectronica: destorySelf may be a sqlobject-ism, but that is also the method we use for all destroys. I do not know of another <intellectronica> sinzui1: the storm way of doing this is store.remove(obj) <intellectronica> or something like that <sinzui1> intellectronica: I can try that <intellectronica> sinzui1: Store.of(obj).remove(obj) <intellectronica> a bit clumsier, but it will be nice to slowly get rid of the sqlobject stuff in the codebase <sinzui1> intellectronica: I am skeptical of moving the product series work into the model. 1) it is not true delete, 2, it couple the model to a dozen objects, some of which are outside the registry <intellectronica> sinzui1: right, i understand the dilemma <intellectronica> how would we solve that if and when we want to expose that functionality via the API? <sinzui1> We do not intend to expose this over the API <sinzui1> When we can delete translations, we ill consider it <intellectronica> that's a good answer, i guess :) <sinzui> intellectronica: danilo thinks it is possible but this kind of issue is not out focus anymore <sinzui> :( <intellectronica> though i can totally see how milestone and series mgmt is something project teams might want to automate <intellectronica> yeah, i realise that :-/ <intellectronica> oh well, hopefully one day we'll have an opportunity to improve on that <intellectronica> sinzui: for now, r=me <sinzui> thanks. I will work on the storm change now <intellectronica> cool * intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica, abentley || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <sinzui> intellectronica: thanks for the storm suggestion. I was able to factor out removeSecurityProxy. <intellectronica> excellent! * intellectronica changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com #launchpad-reviews 2009-10-06 * abentley changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <adeuring> gmb: could you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/hwdb-submission-consistency-check-udev/+merge/12910 ? <gmb> adeuring: Sure. <adeuring> gmb: thanks! * gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: adeuring || queue: [] || This channel is logged: http://irclogs.ubuntu.com <gmb> adeuring: In your tests, you need to tidy up the dicts a bit. For single item dicts you can get away with the "{'id': 1}," style. Otherwise, opening braces should be on the end of the preceding line, not on a line of their own. <adeuring> gmb: right... <gmb> adeuring: Take a look at the dicts in the devices list on line 233 of the diff, for example. <gmb> adeuring: Otherwise, this is r=me. <adeuring> gmb: thanks! <bigjools> gmb: got room for another review? <gmb> bigjools: Sure <bigjools> gar <bigjools> clicking on the wrong person in the chooser is seriously annoying, you don't get a chance to separately OK the review request :/ * bigjools changed the topic of #launchpad-reviews to: on call: gmb || reviewing: adeuring || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com * gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com <gmb> Sampledata change. Nooooooooooooo. <stub> bigjools: Feel like adding the patch-.sql file and pushing? <bigjools> stub: ohhhhhhhhhhhh fuck <bigjools> gmb: lint requires it <bigjools> schema change <gmb> bigjools: Yeah, I know. Wish we could exclude its largeness from the diff though. <stub> bigjools: Is there any foreseeable use of storing the new data in a more structured format? <stub> bigjools: I'm assuming it is just used raw as is by the soyuz code. <gmb> bigjools: Code looks food, fwiw. r=me. <bigjools> stub: no, I see this as a temporary change until OEM has all their archives in LP <bigjools> gmb: grassyass <gmb> bigjools: Day nardar. <bigjools> :) <gmb> West country accented spanish. That sounds like it should be a sitcom trope. <bigjools> stub: it gets passed to the builders as-is, whence it is placed in the sources.list in the chroot <stub> cool. Assuming the patch is just 'ALTER TABLE foo ADD COLUMN bar text;' it all looks fine at my end. <bigjools> yep that's it <bigjools> I pushed the new rev up BTW <stub> bigjools: Is Mark aware of this yet? (Not that I see him having any issues with this one) * gmb really needs to remember to write an X-Chat plugin that manages the review queue. Or a bot. Or something else. * gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <bigjools> stub: I think jml is doing that job now <stub> cool <bigjools> which is why he's on the MP ;) <allenap> jml: Do you have a few minutes to look at https://code.edge.launchpad.net/~allenap/launchpad/packages-for-autoload-bug-443061/+merge/12867 -- it's a patch for the autoland problem we talked about yesterday. <jml> allenap, sure. <al-maisan> jml: I was wondering whether you had a chance to look at lp:~al-maisan/launchpad/dd-bug-no-1 <jml> al-maisan, no, not yet. I plan to today <jml> (just going through a backlog of email caused by sprinting & moving to London) <jml> al-maisan, I did skim it though. I was wondering if you looked at lp:~jml/launchpad/upload-permission-joy at all? <al-maisan> jml: no problem <al-maisan> jml: you mean the refactoring you did? <jml> al-maisan, that's right. <jml> al-maisan, well, the second work-in-progress refactoring branch that I was doing before I changed the branch permission stuff :) <al-maisan> jml: the function added (person_may_edit_branch()) makes use of verify_upload() <al-maisan> the function that resulted from your refactoring <jml> al-maisan, yes, but there's a second & different refactoring branch that's in progress. <al-maisan> jml: oh <al-maisan> would that be 'lp:~jml/launchpad/upload-permission-joy' ? <jml> al-maisan, yes <al-maisan> jml: sorry, I merged lp:~jml/launchpad/upload-permission-joy into LP devel, that was my starting point <al-maisan> or .. wait <al-maisan> did I get the branch wrong .. just a sec.. <jml> al-maisan, I think you got the branch wrong. Your first commit says Imported jml's 'official-write-permissions' branch, and I don't see my work from u-p-j in the diff. <al-maisan> jml: hmm .. I see. <al-maisan> jml: I'm on CHR duty today but will take a look at lp:~jml/launchpad/upload-permission-joy tomorrow .. <al-maisan> maybe we can have a chat then <jml> al-maisan, cool. I look forward to it :) <al-maisan> :) * noodles775 changed the topic of #launchpad-reviews to: on call: gmb || reviewing: - || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com <noodles775> Hi gmb, got time for one? https://code.launchpad.net/~michael.nelson/launchpad/436182-newlines-in-sources-workaround-pt2/+merge/12918 <gmb> noodles775: Sure. <noodles775> Thanks * gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com <gmb> noodles775: Nice branch. r=me * gmb lunches <noodles775> Thanks gmb. * gmb changed the topic of #launchpad-reviews to: on call: gmb || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com * bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: lunch, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <bac> good morning * gmb changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <gmb> Morning bac. <bac> hi graham. quiet review day? <gmb> bac: So far. Let's not tempt fate, eh? <bac> gmb: have you upgraded to karmic? are there expected test failures for the LP suite? <gmb> bac: I've only upgraded my netbook so far; waiting on reports back from deryck about whether the Karmic upgrade affects the development process before doing the work machines. <bac> gmb: i did a netbook last week too. did my dev box yesterday. ran the full suite last night and got a handful of errors <gmb> Not unexpected, I guess. <bac> nope * sinzui changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com <sinzui> gmb: bac: Do either of you have time for https://code.edge.launchpad.net/~sinzui/launchpad/announcement-heading/+merge/12898 <bac> sinzui: just claimed it * bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, sinzui || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com <bac> hi sinzui. i'm looking at your changes to create_view and create_initialized_view <bac> it looks good but i'm surprised you didn't use them in this branch. <sinzui> bac: I did. I used them to test breadcrumbs. I had to create a fake request <bac> sinzui: gah, nm i see it in use at th e last line <bac> sinzui: nice change. r=bac <sinzui> thank * bac changed the topic of #launchpad-reviews to: on call: gmb, bac || reviewing: -, - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com * gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <allenap> bac: Can you review a very small lpbuildbot branch please? https://code.edge.launchpad.net/~allenap/lpbuildbot/avoid-deadlock/+merge/12921 <bac> allenap: yes. i'm about to start a 45 minute call but can look at it afterwards <allenap> bac: Thanks. * allenap changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com <gmb> bac, I'll take allenap's branch * gmb changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <allenap> Thanks gmb. <gmb> allenap: So, can you explain to my addled mind why proc.wait() is safe now that you've grabbed stdout before doing it? <gmb> Cos reading the MP, I don't get it :) <allenap> gmb: proc.wait() waits for the process to finish. If it tried to write more than 64k to its stdout, it would fill the buffer and be blocked on writing, waiting for it to be read, which won't happen because we're waiting for it to finish. <allenap> s/it to be read/the buffer to be read/ <gmb> allenap: Ah right, I think I get it now. * gmb Needs tea <gmb> allenap: r=me, then. <allenap> gmb: Danke :) <gmb> allenap: You'll need to set the branch status manually as I'm not sub'd to it. <allenap> gmb: Strange, ~launchpad is subscribed to the trunk. I wonder if that's a bug. <allenap> gmb: Ah, I can't set it either! <gmb> Hah. <gmb> allenap: That might be a bug then. <gmb> allenap: While you're here, do you ahve time for a reciprocal review? <gmb> It's 300 lines, but it's mostly refactoring <allenap> gmb: Or some new Krypton Factoresque puzzle from the code crew. <allenap> gmb: Yeah, sure. <gmb> allenap: https://code.edge.launchpad.net/~gmb/launchpad/checkwatches-all-to-reset-bug-442993/+merge/12930 <gmb> allenap: It's a fix for bug 442993, not 422993 as the MP suggests. <mup> Bug #442993: checkwatches --all should be --reset <bugwatch> <Launchpad Bugs:Triaged> <https://launchpad.net/bugs/442993> <allenap> gmb: I was looking at the bug thinking "whaaa?" <gmb> Indeed. <gmb> What amuses me is that someone understood that bug well enough to mark it as a dupe. <jml> allenap, references to the Krypton Factor were one of the many things in "A Bit of Fry and Laurie" that I didn't get. <bac> gmb: thanks for taking allenap's review. i'm off the call now <allenap> jml: If those two chose to be cryptic, who am I to let you in? I mean, Stephen Fry has recently been described as the "King of Norfolk" no less. <jml> allenap, well, it's more that the Krypton Factor never existed in .au <jml> allenap, so jokes about it being unwatchable lack punch. <leonardr> bac, i'll have a fun review for you in a few minutes <bac> leonardr: ok <leonardr> bac: here you go: https://code.edge.launchpad.net/~leonardr/lazr.restful/oauth/+merge/12931 * bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com <jml> https://code.edge.launchpad.net/~jml/launchpad/browse-source-code/+merge/12933 when you're ready :) <jml> beuno-lunch, ^^^ <allenap> gmb: r=me <jml> bac, can you please review that branch? <bac> jml: sure. after leonard's and lunch <jml> bac, thanks. <bac> or maybe lunch then leonard's <gmb> allenap: Awesome, thanks. <gmb> allenap: Can you mark the mp approved please for the sake of ec2 land? <gmb> Not that I can't do it myself, but that still seems funky to me. <allenap> gmb: Sure, sorry, I forgot. <gmb> allenap: I honestly think that if there's only one review the reviewer should be presented with that option when they approve the review. But maybe that's just me. * gmb will file a bug. <allenap> gmb: Yeah, it's a two-step process that I'm often doing. I think the option to default to "no change", but it would be useful if it were there. <allenap> s/to default/should default/ <gmb> allenap: Better still, it would happen automagically. After all, if there are n reviewers (where n > 1), who gets to decide that the whole thing is ready to land? The last reviewer? What if they're just reviewing the legal aspects of the change... etc. <jml> gmb, ec2 land also has a "--force" option to set the overall mp status. <jml> well, not set, but ignore. <jml> but it needs actual votes to populate the thingummy. <gmb> jml: If anyone ever provides a --force option for a thing, I decline to use it, in case it will do something Bad. Even if I know it won't. <allenap> gmb: How about a --go-on option? <gmb> But maybe that's a hangover from my old job where we had a script that needed --force, --really-force and --no-seriously in order to actually do anything useful. <gmb> allenap: --pretty-please <jml> gmb, fair call. <jml> gmb, that's kind of the spirit that I added it in (i.e. don't actually use it) <gmb> Glad to hear it :) <jml> but yes, we need a much better way of handling the overall mp status <gmb> jml: Incidentally, `ec2 land` is made entirely out of win. Nice job :) <jml> gmb, thanks. <bac> leonardr: nice branch, thanks. * bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: jml || queue: [] || This channel is logged: http://irclogs.ubuntu.com <bac> jml: done. thanks <jml> bac, thank you. <jml> you wouldn't happen to know the answer to my question in #launchpad-dev, perchance <leonardr> bac: ok, i hope to have a corresponding lazr.restfulclient branch for you later today <bac> leonardr: look forward to it <bac> jml: for ec2 land, what does the commit message need to look like? what parts does the script fill in for you? will you update the message on your MP i just reviewed? <jml> bac, Just the text. ec2 fills in the square bracket bits. no, I'll just use --commit-text. <jml> bac, e.g. ./utilities/ec2 land https://code.edge.launchpad.net/~jml/launchpad/browse-source-code/+merge/12933 --commit-text="Change the link to browse source code to point at actual source code, not a revision log." <bac> jml: nice. <jml> there's a --dry-run option to let you muck around. <jml> output of that command + dryrun is: ['./utilities/ec2', 'test', '--headless', zstrickland@example.net', '-b', 'launchpad=devel', '-s', u'[r=bac][ui=beuno][bug=369273] Change the link to browse source code to point at actual source code, not a revision log.', 'bzr+ssh://bazaar.launchpad.net/~jml/launchpad/browse-source-code'] <bac> ping rockstar <gary_poster> allenap: I have a fix for the problem that https://code.edge.launchpad.net/~allenap/launchpad/packages-for-autoload-bug-443061/+merge/12867 addresses <gary_poster> AFAIK <gary_poster> So that you would no longer need to warn people about that problem <gary_poster> maxb: fwiw, ^^^ <maxb> whoa, your commit message has broken the layout of the BMP page ! :-) <gary_poster> allenap: I'll add a comment to the MP. If this diff has landed, that's fine. It won't cause any harm, and we can revert it later <rockstar> bac, hi <bac> rockstar: hi <rockstar> bac, what's up? <leonardr> bac: here's the lazr.restfulclient branch: https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/oauth/+merge/12940 <bac> rockstar: i was trying to use branch.getMergeProposals() from launchpadlib and am not ever getting any results. i wonder if you've used it. <rockstar> bac, tarmac uses it. * bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com <bac> leonardr: will start now <leonardr> great <rockstar> bac, are you sure you have the right branch? <bac> In [54]: br <bac> Out[54]: <branch at https://api.edge.launchpad.net/beta/~thumper/launchpad/code-review-history> <rockstar> bac, lemme look. <bac> thx <bac> nice leonardr <bac> rockstar: i see what is happening now <rockstar> bac, yes? <bac> branch.getMergeProposals returns MPs proposed to merge INTO the branch not ones for where the branch is the source <rockstar> bac, yeah, that's what my hypothesis was, but I couldn't confirm it yet, as I was on the phone. <bac> rockstar: the good news is there is branch.landing_targets <rockstar> bac, yeah, that's what I was going to suggest, and that getMergeProposals should return all involved proposals. <rockstar> We seem to have some redundancy there. Could you file a bug on that? <bac> rockstar: is there redundancy or just poor documentation? <rockstar> bac, redundancy, as we also have branch.landing_candidates <bac> rockstar: oh, i hadn't looked at that one yet <bac> rockstar: sure, i'll open a bug <rockstar> bac, thanks. <bac> rockstar: though landing_candidates only returns those in a final state where getMP allows you to specify the status. just saying there is some varied functionality <bac> rockstar: bug 444854 <mup> Bug #444854: API for getting a branch's merge proposals is confusing <Launchpad Bazaar Integration:New> <https://launchpad.net/bugs/444854> <rockstar> bac, yeah, I think that bug reflects the real problem: confusion. <rockstar> bac, got time for at least one little branch, maybe two? <bac> rockstar: at least one <bac> rockstar: branches for me? <rockstar> bac, one coming up. Sorry, learned another difference between lightweight and heavyweight checkouts. <bac> rockstar: np <rockstar> bac, https://code.edge.launchpad.net/~rockstar/launchpad/branch-upgrade-job/+merge/12945 <mwhudson> bac: you there still? <bac> hi mwhudson * bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <mwhudson> bac: i have a fix for a very simple bug <mwhudson> bac: but the test is offensive <bac> great <mwhudson> bac: here's the diff http://pastebin.ubuntu.com/287338/ <mwhudson> (this is the bug https://bugs.edge.launchpad.net/launchpad-code/+bug/444387) <mup> Bug #444387: productseries-codesummary.pt typo gives all series branches 31 recent revisions <post-ui-3-cleanup> <trivial> <Launchpad Bazaar Integration:Triaged> <https://launchpad.net/bugs/444387> <mwhudson> bac: any ideas for how to make the test less horrid? * bac looks <bac> mwhudson: sorry, i can't think of how to simplify that test <mwhudson> bac: oh well, i guess i'll just try to hide it with some bland prose <bac> mwhudson: the narrative at line 32 of your diff is a bit perplexing. Could you expand that a bit? <mwhudson> bac: heh, yes, that bit isn't finished yet... <bac> ok <bac> mwhudson: based on what you have you can rs=bac if you can't find a reviewer later. <mwhudson> bac: thanks <mwhudson> bac: how long are you around for? <bac> not long at all <bac> i may be back later, though <bac> but i'm checking out as OCR now * bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <mwhudson> ok <barry> bac: ping <bac> barry: this change looks great. r=bac http://pastebin.ubuntu.com/287348/ <barry> bac: thanks! <mwhudson> bac: http://pastebin.ubuntu.com/287352/ diff now looks like this, have time to review it still? <mwhudson> rockstar: hi, can you do a simple review for me? <mwhudson> rockstar: https://code.edge.launchpad.net/~mwhudson/launchpad/bug-444387/+merge/12949 <rockstar> mwhudson, looking. <mwhudson> rockstar: thanks <rockstar> mwhudson, r=me. Thanks for getting us some more test coverage. <mwhudson> rockstar: thanks <mwhudson> rockstar: can i bang another simple one past you? <mwhudson> rockstar: https://code.edge.launchpad.net/~mwhudson/launchpad/rocketfuel-setup-ssh/+merge/12950 <rockstar> mwhudson, r=me <mwhudson> rockstar: thanks #launchpad-reviews 2009-10-07 <rockstar> mwhudson, going to find dinner, will be back to handle any more reviews in just a bit. <mwhudson> rockstar: i think i'm done with the trivial stuff for now, thanks <al-maisan> Good morning! <stub> Trivial review needed to close a 4 digit bug: https://code.edge.launchpad.net/~stub/launchpad/bug-3050/+merge/12977 <jml> stub, already done. <stub> jml: Just saw that :) ec2test already run so its just pqm submit for this one. <jml> stub, ok, thanks. * allenap changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com <stub> jtv: np. Do you need me to land any of my branch or are you still going to be working with 8.3 for the time being? <jtv> stub: No need; I left the issues that your branch resolves untouched, and that avoids any conflicts. * noodles775 changed the topic of #launchpad-reviews to: on call: allenap || reviewing: - || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com <noodles775> Hi allenap - a really easy one for you: https://code.launchpad.net/~michael.nelson/launchpad/add-soyuz-windmill-layer/+merge/12983 <noodles775> (well, short :) ). <allenap> noodles775: On it :) <noodles775> Thanks! * allenap
{'channel': 'launchpad-reviews', 'month': 10}