#launchpad-reviews 2010-03-22
* NCommander changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [NCommander] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* wgrant changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> Oops.
* wgrant changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [NCommander,wgrant] || 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: [NCommander,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* StevenK changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: [NCommander,wgrant,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> NCommander: is your change really that big, or is the diff in the MP wrong?
<intellectronica> i see most of it is sample data changes
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: wgrant || queue: [NCommander,StevenK] || 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: StevenK || queue: [NCommander] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> intellectronica: Thanks. Can you land it?
<intellectronica> wgrant: yup, submitting it for tests now.
<StevenK> intellectronica: https://code.edge.launchpad.net/~stevenk/launchpad/fixes-bug-503258/+merge/21710 is the MP
<intellectronica> StevenK: on it
<wgrant> intellectronica: Thanks.
* bigjools changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: StevenK || queue: [NCommander, bigjools (update-sourcecode fix)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> wgrant: why is your merge target db-devel? do you depend on anything not in devel?
<wgrant> intellectronica: All the previous work is still only in db-devel.
<wgrant> (there was a new table required)
<intellectronica> ok
<intellectronica> StevenK: why the call to debug in lib/lp/soyuz/model/queue.py? is that a leftover, or something you really want merged?
<StevenK> intellectronica: It matches the previous call -- the bit I added is just for custom files associated with the upload
<intellectronica> StevenK: also, the import in packagecopier.py is not alphabetically sorted. if you're using emacs or vi, b.t.w, there are helpers on the wiki to make sorting easy
<intellectronica> ok
<StevenK> intellectronica: Right, I can re-order them
<intellectronica> cool
<intellectronica> StevenK: you can use assertFalse instead of assertEquals(False, ... no?
<StevenK> intellectronica: My unit testing memory is a little rusty, I'll do that too
<intellectronica> StevenK: great. other than these two comments the branch is all goodness. make the changes and ping me and i'll land it.
<StevenK> intellectronica: Changes pushed.
<intellectronica> StevenK: lovely, thanks
<StevenK> intellectronica: Thank you :-)
<intellectronica> StevenK: oh, can you also please set the commit message?
<StevenK> intellectronica: Done
<intellectronica> cheers
<StevenK> intellectronica: I'll land it after bigjools sorts out access
<intellectronica> StevenK: oh, even better :)
 * StevenK coughs in bigjools's general direction ;-)
 * bigjools nods
<wgrant> StevenK: I wonder if you should XXX the security change? Details on hacks are handy years later.
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: bigjools || queue: [NCommander] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> bigjools: r=me
<bigjools> intellectronica: easy one huh? :)
<bigjools> thanks
<intellectronica> nothing's easy on a monday morning
<noodles775> intellectronica: another one for you when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/fix-buildd-slave-test/+merge/21847
* noodles775 changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: bigjools || queue: [NCommander, noodles775] || 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: noodles775 || queue: [NCommander] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> noodles775: sure. also, can i bother you now for that ui review? should be pretty straightforward.
<noodles775> intellectronica: yeah, that'd be great... I'll get mine ready for you too (a UI review).
<intellectronica> cool
<intellectronica> noodles775: https://code.edge.launchpad.net/~intellectronica/launchpad/bug-531963/+merge/21628 for my ui review
<intellectronica> noodles775: and r=me on your branch
<noodles775> Thanks intellectronica.
* intellectronica changed the topic of #launchpad-reviews to: on call: intellectronica || reviewing: - || queue: [NCommander] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> intellectronica: why is it that if I'm logged in as cprov (celso.providelo@canonical.com:cprov) I'm unable to modify the bug status (the mouse-over tells me "Changeable only by a project maintainer or a bug supervisor"), yet I can as no-privs? Is no-privs a bug supervisor without privs in the project?
<intellectronica> noodles775: are you sure that's status and not importance?
 * noodles775 checks
<intellectronica> let me see if i can get that effect locally
<noodles775> intellectronica: right, the mouseover is from the importance icon, but I still can't change the status as Celso, but can as no-privs?
<intellectronica> noodles775: cprov and no-priv behave the same for me. you can't change the importance, you can change the status, but asked to confirm.
<noodles775> intellectronica: wierd... this is the bug I'm looking at: https://bugs.launchpad.dev/thunderbird/+bug/15
<mup> Bug #15: PO file import errors should be more verbose <feature> <Launchpad Translations:Fix Released by carlos> <https://launchpad.net/bugs/15>
<noodles775> intellectronica: on that page, clicking on the status for Mozilla Thunderbird always redirects to the +editstatus page, whereas Redfish works as expected.
<intellectronica> noodles775: that's because that bug is assigned to a bugwatch
<noodles775> Ah ok. Thanks. Two more things:
<intellectronica> bugs that are tracked by a bugwatch can't have their metadata set by users
<noodles775> First, I think the wording might be missing "whether"? eg. "If you're not certain whether changing the status of this bug is correct,..." What you you think?
<noodles775> And second, how difficult would it be to first close the overlay before popping up the confirmation?
<intellectronica> noodles775: as a non-native speaker i'll have to trust you on that. both some equally good to me
<noodles775> I think that's quite important (a popup within a popup is really bad).
<intellectronica> noodles775: very difficult
<noodles775> :(
<noodles775> intellectronica: couldn't in just be hidden with CSS when you bring up the confirmation?
<noodles775> (ie. you shouldn't need to change the behaviour of the widget?)
<intellectronica> that would be a great solution, and even better one would be to handle it within the widget itself. unfortunately both require a complete restructuring of that widget which we can't afford right now
<intellectronica> noodles775: the problem is the way the widget interacts with the web service. it saves before closing, and changing that would require changing the widget, and quite possibly changing other widgets that use the api patch plugin
 * intellectronica --> lunch
* abentley changed the topic of #launchpad-reviews to:  on call: intellectronica, abentley || reviewing: -,- || queue: [NCommander] || 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: intellectronica, abentley || reviewing: -,- || queue: [NCommander,jpds] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> EdwinGrubbs: hi. I have updated my branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-540105/+merge/21552
<adiroiban> it looks like the try/catch can still act as sandboxes
<EdwinGrubbs> adiroiban: I'll check it after I get off of a call.
<jml> does someone want to review https://code.edge.launchpad.net/~jml/launchpad/visible-dependencies/+merge/21807 ?
<intellectronica> jml: sure, i'll review it
<jml> intellectronica, thanks.
<EdwinGrubbs> adiroiban: I'm wondering how you were able to cause some of the domready handlers to fail to run. This test page worked fine in chrome and firefox. http://pastebin.ubuntu.com/399378
<EdwinGrubbs> adiroiban: I started looking at pofile.js to see if there was anything in there that would cause one error to affect another function. I didn't find anything conclusive, but I did find some global variables that should be made local, and that could have unexpected results.
<adiroiban> http://paste.ubuntu.com/399381/
<adiroiban> without try/catch, the Y.lp.pofile.initializeKeyBindings(); is not  called
<adiroiban> EdwinGrubbs: if you think the try/catch are not needed, I can remove them :)
<EdwinGrubbs> adiroiban: I am able to confirm the problem you were getting, but I'm still trying to understand why this is not happening in my simple example. I'll let you know in a few minutes.
<EdwinGrubbs> adiroiban: while you are waiting, here are some problems in pofile.js that could lead to very hard to diagnose errors. http://pastebin.ubuntu.com/399412
<EdwinGrubbs> adiroiban: ok, I think I've figured out why my example yielded different results. Since the html was so simple, the dom was immediately ready. Therefore, Y.on(domready) would immediately run the handler and the exception would cancel anything that was already registered, but there wasn't anything.
<EdwinGrubbs> then, the next Y.on(domready) would get called from the html file and run the handler immediately.
* intellectronica changed the topic of #launchpad-reviews to: on call: abentley || reviewing: - || queue: [NCommander,jpds] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> adiroiban: therefore, your try/catch blocks are perfectly fine. However, there is no reason to have them in separate Y.on('domready') blocks, so you can combine them into a single Y.on(domready)
<adiroiban> EdwinGrubbs: right. I will put them in a singly 'domready' block and will fix the errors from your previous pastebin. I'm just running the tests and then I will push the changes. Thanks!
#launchpad-reviews 2010-03-23
<NCommander> anyone around?
 * thumper hides
<NCommander> thumper: can I tempt you for a review? :-)?
<thumper> NCommander: not right now, I'm just about to go make dinner and come back later this evening
<NCommander> thumper: fair enough
<mwhudson> NCommander: i might be able to have a look at it
<NCommander> mwhudson: cool!
<NCommander> mwhudson: https://code.edge.launchpad.net/~mcasadevall/launchpad/raw_source_changelog_librarian - it probably needs another ec2test run but I fixed all the failures that were in the email
<mwhudson> NCommander: did you know that there's a theoretical limit of 800 lines of diff for a review?
<mwhudson> oh i see, a lot of the diff is sampledata changes
<NCommander> mwhudson: no I didn't, although the wiki says to ignore the sampledata change size :-/
<mwhudson> NCommander: it's getting a bit late for me, but does your branch store the changelog in the database or in the librarian?
<NCommander> mwhudson: librarian. See dscfile.py changes
<NCommander> mwhudson: findAndMoveCopyright and the self.librarian.create call
<mwhudson> NCommander: then the sampledata changes look strange
<mwhudson> as they seem to include big chunks of changelog
<mwhudson> NCommander: do you need to regenerate the sampledata again?
<NCommander> mwhudson: I see a whole lot of NULLs for the pointer to LFA. There is some changelog data stored in the database by the existing code however
<NCommander> mwhudson: thats going to get depericated and eventually removed with a little luck
<mwhudson> oh ok
<mwhudson> ahh, happiness is meld on a bigger monitor
<NCommander> mwhudson: that's the changelog_entry your probably seeing, and there are quite a few issues with its design sadly, raw source changelog will hopefully let us replace it -)
<NCommander> meld?
<mwhudson> NCommander: a diff tool
<NCommander> mwhudson: I'm sorry to keep you awake :-/
<mwhudson> aagh soyuz code it burns the eyes
<mwhudson> NCommander: it'
<mwhudson> s only late to be concentrating, not to be awake :)
<NCommander> mwhudson: heh, I plan to be writing more soyuz code in the future, so make sure the brian bleach is near by
<mwhudson> createUploadedSourcePackageRelease takes far too many arguments
<NCommander> mwhudson: no kidding :-/
<mwhudson> NCommander: ah right, i've finally found the interesting parts of your change
<NCommander> mwhudson: I'll be AFK for about 15 minutes
<mwhudson> NCommander: it'll probably be that long before you have a review, but thanks for letting me know
<NCommander> mwhudson: back
<mwhudson> NCommander: still going!
<mwhudson> NCommander: review submitted
<NCommander> mwhudson: thanks. Fixing.
<NCommander> mwhudson: maybe yo ucan explain when a module should be in __all__
<mwhudson> NCommander: well, maybe that's overly picky of me
<mwhudson> NCommander: one way around is that it _needs_ to be in all if it's imported in another module
<mwhudson> NCommander: i guess the question is, is findFile part of the 'public interface' of the module
<mwhudson> it's a close call in this case
<NCommander> mwhudson: I think its used in the test suite
<NCommander> mwhudson: ugh, a few of these exist just before I didn't properly clean up with all the refactoring this branch has gone through
 * NCommander headthunks
<mwhudson> NCommander: that's what reviews are for :-)
<NCommander> mwhudson: with 10MiB changelog, that was a security fix that was discovered when I was writing the code
<NCommander> mwhudson: bigjools and I threw some numbers, and we decided 10MiB was a good value. The old value was 2GiB (hardset DB limit)
<mwhudson> NCommander: ok
<mwhudson> NCommander: i remember some discussion
<mwhudson> but i remembered it being about copyright, not copyright and changelog
<mwhudson> if i remembered wrong, that's fine :)
<NCommander> mwhudson: well, that's from an earlier version of the branch where changelog and copyright were in the database
<NCommander> changelog will now be in librarian instead of the DB, so I could actually drop that check
<NCommander> mwhudson: or, well, move it to debian/copyright's finder function
<mwhudson> NCommander: in this case i'm just asking the question
<mwhudson> making sure you've thought it through
<mwhudson> i don't have the answer :)
<NCommander> mwhudson: heh :-)
<NCommander> mwhudson: you can't set changelog_path with dsc_file's path since its a temporary directory :-/
 * NCommander was greatly annoyed by having to make that interface change
<mwhudson> NCommander: i mean you could say dsc_file.changelog_path = foo then call findAndMoveChangelog(dsc_file)
<NCommander> mwhudson: ah, I could do that, although then I need to whack the testing code a bit
<NCommander> mwhudson: (I sorta like to keep my functions self contained, but maybe just because I'm weird ...)
<mwhudson> NCommander: maybe leave it
<mwhudson> NCommander: i'm off to sort out dinner, i'll be around ish for another 30 mins maybe but perhaps you'll need to get someone else to review the branch again
<NCommander> mwhudson: I'm almost done fixing most of the issues
<NCommander> mwhudson: pushed
<NCommander> (and resubmitted)
<mwhudson> NCommander: you don't need to resubmit
<NCommander> mwhudson: oh ...
 * NCommander already did :-/
<mwhudson> well you'll know for next time :)
<mwhudson> NCommander: sorry have to go now, i'll review it first thing tomorrow if noone else gets to it
<NCommander> mwhudson: fair enough
<StevenK> NCommander: If you need to make changes to a branch, just push it, the MP is clever enough to realise the branch changed
<NCommander> StevenK: I hit the resubmit button so mwhudson Needs Fixing goes back to Review
<StevenK> NCommander: You can edit the MP state too ...
<NCommander> StevenK: ah
<StevenK> NCommander: I can edit my own MPs and set them betwen WIP, Needs Review and Merged. I'm unsure why the last one is allowed, but the first two make perfect sense.
<NCommander> StevenK: are you in ~launchpad?
<NCommander> StevenK: I can see Merged, but I always thought that's because I'm in ~launchpad
<StevenK> I don't think so.
<StevenK> Confirmed, I'm not a member of ~launchpad
<wgrant> StevenK: What harm is there in letting people set their own MPs as Merged?
<wgrant> It's useful if it wasn't automatically detected.
<StevenK> Well, what happens if an MP is set to Merged before it's even reviewed?
<wgrant> It drops off the review list.
<StevenK> IE, it could just drop through the cracks if the submitter just sets it to Merged?
<wgrant> Yes.
<wgrant> But if they do that, then they're pretty silly.
<wgrant> It's just like if they set it back to Work in Progress
<StevenK> I've done that, which is useful
<StevenK> "I thought I was ready, oh damn, I missed x, y and z."
<NCommander> StevenK: file a bug?
<gmb> stub, BjornT: Could you take a look at https://code.edge.launchpad.net/~gmb/launchpad/bugwatch-next_check-bug-544238 please? Really need to get this one landed as soon as possible.
<stub> nag nag nag... :)
<gmb> :)
<gmb> stub: Thanks for the review :)
<NCommander> stub: BTW, mind looking at the revised raw source changelog (which now uses librarian when you get a chance?)
<NCommander> morning bigjools
<gmb> jml: Can I have your vote on https://code.edge.launchpad.net/~gmb/launchpad/bugwatch-next_check-bug-544238 please?
<BjornT> gmb: sure, i'll look at it, if you explain how it will be used in more detail :)
<gmb> BjornT: Cool. So, we want to be a bit more sensible with scheduling bug watch checks. At the moment we check each watch once every 24 hours, but if it's erroring a lot (which we now record in the BugWatchActivity table) we don't want to check it so often.
<gmb> BjornT: So, instead of building that logic into BugTracker.getBugWatchesNeedingUpdate(), which would be complex at best, we think we should have a garbo-hourly process that looks at all the bug watches checked in the last hour, checks to see how often they've errored and then schedules their next_check accordingly.
<gmb> So BugTracker.getBugWatchesNeedingUpdate() will look at the next_check field instead of lastchecked to determine whether a watch needs checking.
<gmb> The advantage is that we don't have to further complicate checkwatches to get the back-off for erroring watches.
<BjornT> gmb: ok, that makes sense. i'm wondering if we could find a better name? if you set next_check to some time, it doesn't mean that it will be checked exactly at that time, will it?
<gmb> BjornT: Hmm, true. How about check_after?
<stub> check_due ?
 * stub gets out his bike shed schematics
<gmb> It should be blue.
<BjornT> gmb: maybe next_check is good, as long as there is a comment explaining the semantics. can't think of a better name, and i think next_check is better than the other suggestions.
<gmb> BjornT: Right; I'll add the comment (should've added it anyway).
<NCommander> BjornT: you around?
<BjornT> NCommander: kind of. i'm in desperate need of lunch, though :)
<NCommander> BjornT: ah, no rush :-)
<NCommander> BjornT: if after lunch you'd be willing to look at my raw_source_changelog_librarian branch, I'd be most grateful
<NCommander> stub: thanks for the review
<BjornT> NCommander: sure. i have a bit of a backlog returning from vacation, but i'll try to get at it today
<NCommander> BjornT: thanks
<NCommander> BjornT: (its a new merge proposal versus the old one)
<noodles775> intellectronica: you don't think it's worth closing the choice edit before bringing up the confirmation?
<noodles775> (for https://code.edge.launchpad.net/~intellectronica/launchpad/bug-531963/+merge/21628)
<intellectronica> noodles775: i do. thanks for your patch, that's exactly what i was missing :)
<intellectronica> i didn't consider using css to get rid of it
<noodles775> intellectronica: ah, I just didn't see it in the MP diff. Great.
<jml> gmb, I'm confused
<jml> gmb, do you want me to do a code review or a db review?
<gmb> jml: Sorry, DB; typo when I requested the review.
<jml> gmb, ta
<NCommander> anyone around to do a code review?
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [NCommander,jpds] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> hi NCommander
<NCommander> morning bac
<NCommander> bac: https://code.edge.launchpad.net/~mcasadevall/launchpad/raw_source_changelog_librarian/+merge/21909
<bac> NCommander: morning.  i'll take a look.
<NCommander> bac: thanks:-)
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing:  ncommander|| queue: [jpds] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> NCommander: i see that branch has already been reviewed by michael.  generally, for consistency, we have the original reviewer look at changes.
<NCommander> bac: his comment he made earlier tonight was to have someone else look at it, else he'd look at it tonight
<NCommander> bac: 01:57:31 < mwhudson> NCommander: sorry have to go now, i'll review it first thing tomorrow if noone else gets to it
<bac> NCommander: ah, ok
<NCommander> bac: sorry for the confusion
<bac> NCommander: and is the branch really 2600 lines?
<NCommander> bac: no, its about ~100-200, but I regenerated sample data
<bac> ah, ok.  thanks for that
 * bac looks
 * NCommander hands bac the brain-bleach
<bac> hi NCommander -- i've updated your MP.  just a few fixes and then it'll be ready to land.  thanks a ton for the branch!
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing:  jpds|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<NCommander> bac: I still need a BjornT +1 before it can land :-/
<bac> NCommander: right
<NCommander> bac: ugh, I missed  afew changes. I shouldn't resubmit on lack of sleep >.<;
<bac> NCommander: np
<bac> jpds: i don't see your MP on https://code.edge.launchpad.net/launchpad/+activereviews
<jpds> bac: I think bigjools is dealing with it.
<bigjools> yes
* bac 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
<bac> jpds: ok, great.  please remove your name from the topic if you line up a reviewer.
<NCommander> bac: I got everything fixed except one lint error that wasn't in code I modified. Its too long by one character, but I don't see a good way to break this up to multiple lines
<NCommander>             DistroSeries.id == POTemplate.distroseriesID).config(distinct=True)
<NCommander> bac: suggestions welcome
<bac> NCommander: what file?
<NCommander> bac: lib/lp/registry/model/distroseries.py
<NCommander> bac: sorry :-/
<bac> NCommander: you could drop the .config() and put it on the next line as a separate statement
<bac> kind of cheesy.
<NCommander> bac: er, I can?
 * NCommander isn't sure he's reading this right them, or why there is a == on that line
<NCommander> bac: oh, I can. bugh, brain without coffee isn't good
<adiroiban> henninge: hi, do you have time to look at https://code.edge.launchpad.net/~adiroiban/launchpad/bug-201749/+merge/21250 ?
<henninge> adiroiban: maybe later, I am at a sprint atm.
<adiroiban> henninge: ok. no problem!
<NCommander> bac: so, I fixed everything, except utilities/paste doesn't work for me (it wants a python module which I'mlooking for now)
<bac> NCommander: you can do it by hand.  was just a suggestion
<NCommander> bac: ah, ok
<bac> NCommander: but i'd be interested to hear why it doesn't work.  you do have launchpad-dev-dependencies installed?
<NCommander> bac: I do, python-clientcookie wasn't installed
<NCommander> bac: changes made, incremental diff posted. I think I addressed all of mwh and your concerns
<NCommander> [topic] on call: bac || reviewing:  -|| queue: [NCommander] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<NCommander> er
* NCommander changed the topic of #launchpad-reviews to: on call: bac || reviewing:  -|| queue: [NCommander] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> rockstar: hi, fancy a UI pre-imp check?
<bac> NCommander: approved with one tiny change.  thanks
* bac 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
<NCommander> bac: eeek, oops. Sorry. I ran that test suite, nothing blew up :-/
<NCommander> BjornT: how goes your backlog?
<BjornT> NCommander: slowly!
<NCommander> BjornT: ow :-/
<danilos> bac, hi, do you have time to take a look at https://code.edge.launchpad.net/~danilo/launchpad/bug-545070/+merge/21938? (very simple terminology change)
<bac> danilos: yep
<danilos> bac, thanks
<NCommander> BjornT: if you don't have time to look at my branch today, no rush. Its a pretty simple change (one row addition, forgein key on LFA), and its not a huge priority
<bac> danilos: r=bac
<danilos> bac, thanks
<BjornT> NCommander: oh, why didn't you say it was a simple change? :) approved
<NCommander> BjornT: awesome. Now how do I get it landed? :-)
<rockstar> bigjools, sure!
<bigjools> rockstar: great! https://code.edge.launchpad.net/~julian-edwards/launchpad/ppa-deletion-ui/+merge/21925
<BjornT> NCommander: if you ask bac, he might be kind enough to help you :)
<bigjools> rockstar: there's no tests yet, just the basic implementation
<bigjools> but feel free to have a play
<NCommander> BjornT: should the status be set to Needs merge versus Needs Review?
<rockstar> bigjools, if it's UI, I don't care about tests.  :)
<bigjools> rockstar: attaboy
<bac> NCommander: are all of your changes done and pushed?
<NCommander> bac: yes.
<bac> NCommander: ok, i'll send it through ec2 and land it for you
<NCommander> bac: w00t
<rockstar> bigjools, so, I don't feel like I know enough about Soyuz to really grok what I'm trying to review here (and there's no movie/screen shot).  How can I get to the view?
<bigjools> rockstar: go to http://launchpad.dev/~cprov/+archive/ppa
<bigjools> sorry I assume too much :)
<rockstar> bigjools, :)  It's okay.
<rockstar> bigjools, so, once a PPA is requested for deletion, can it be uploaded to?
<rockstar> Also, "Delete PPA" shouldn't show if the deletion request has already been made.
<bigjools> OTP, will type when I can :)
<rockstar> I also wonder if a red notification for "Deletion in progress" is probably better, since it's more likely to grab your attention.
<rockstar> Although the red often means "It's broken. It's broken! It's BROKEN!!!!"
<EdwinGrubbs> bac: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-538469-invalid-upstream-link/+merge/21947
<bac> EdwinGrubbs: yes
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing:  edwin|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> rockstar: ok sorry let me read backscroll
<bigjools> rockstar: gnargh too many distractions
<bigjools> rockstar: to answer your questions, no you can't upload to it, the browser code disables it for that reason
<bac> EdwinGrubbs: nice.  r=bac
<EdwinGrubbs> thanks
<rockstar> bigjools, okay.  Did you see my other suggestions?
<bigjools> rockstar: making it red?
<bigjools> yeah
<bigjools> how do I do that?
* bac changed the topic of #launchpad-reviews to: on call: bac-lunch || reviewing:  -|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<NCommander> bac: so, stupid question, is my branch on EC2, buildbot, or? (I'm kinda confused on how trees actually land ... or how PQM works)
<bigjools> rockstar: what's the best way of making that notification box red?
<rockstar> bigjools, I think that there's a similar method to the info notification method that does red boxes.
<bigjools> perfect
<rockstar> bigjools, also, the "Delete PPA" link shouldn't show if the deletion has already been requested.
<bigjools> rockstar: I thought about that, it was easier to leave it and make the page template different!
<bigjools> generally I really hate links appearing and disappearing with no explanation :/
<bigjools> but I can change it
<bigjools> one day I hope we'll present disabled links
<rockstar> bigjools, grey it out then?
<bigjools> rockstar: does Link do that?
<rockstar> bigjools, I don't think so, but it should...  :)
<bigjools> heh
<rockstar> bigjools, basically, you shouldn't be able to click it if it's no longer deletable (because it's already deleted)
<bigjools> I didn't think about it too hard because it won't be around longer than 5 minutes
<rockstar> http://validator.w3.org/feed/check.cgi?url=http%3A%2F%2Fplanet.nclug.org%2Frss20.xml
<bigjools> in any case we're changing tack slightly, and only removing the repository, the PPA will be disabled and hidden
 * rockstar slaps his middle click
<rockstar> bigjools, yeah, I figured that, but to the user, it's "deleted"
<bigjools> rockstar: yeah
<bigjools> rockstar: so I'll make the whole thing disappear and redirect  to the user's profile page, what do you think?
<rockstar> bigjools, yeah, that's probably a good idea.
<rockstar> And then the info notification is something like "The PPA deletion has been requested."
<bigjools> right
<bigjools> in red?
<bigjools> :)
<rockstar> bigjools, no, at that point, blue is fine.
<bigjools> rockstar: cool, thanks for the pre-imp
<rockstar> bigjools, no prob.
<bigjools> rockstar: I'll ping you for a review when it's done, apparently you need the experience :)
<rockstar> bigjools, yes, I'm only a lowly padawan.
<rockstar> One day I'll be a great Jedi warrior.
<rockstar> Either that, or become Darth Vader.
<rockstar> Depends on whether or not I fall in this river of lava.
<bigjools> These aren't the notifications you're looking for
 * bigjools waves hand
<bigjools> my font made me read that as river of Java
<bac> NCommander: can i get you to set the commit message on https://code.edge.launchpad.net/~mcasadevall/launchpad/raw_source_changelog_librarian/+merge/21909
* bac 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
* bac 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
<NCommander> bac: done
<NCommander> bac: did ec2 pass?
<bac> NCommander: haven't sent it off yet.  there is a bug in our ec2 script.  will do so now that you've set the commit msg
<leonardr> bac: very small branch https://code.edge.launchpad.net/~leonardr/launchpad/add-mutators/+merge/21965
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing:  leonardr || 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:  - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> leonardr: done
<leonardr> bac, great
<bac> NCommander: i've sent your branch off to ec2 with 'ec2 land'.  if the tests pass it will go directly to pqm.  either way you'll get email
<bac> well, theoretically
<bac> abentley: thanks for your email yesterday about the pitiful state of +activereviews
<abentley> bac, :-)
<mwhudson> bac: are you running NCommander's branch through ec2 ?
 * mwhudson reads backlog
<mwhudson> bac: nm
<bac> mwhudson: yeah, i just started it
<bac> mwhudson: first attempt failed
* 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
<bac> NCommander: i sent your branch off to ec2 twice and if failed both times.  it didn't send email, though, so i'm not sure what the problem is
<bac> NCommander: i'm running your tests locally
<NCommander> bac: ugh, now what? :-/
<bac> see if i can get output locally
 * NCommander feels like he really broke LP :-/
<mwhudson> can someone stamp https://code.edge.launchpad.net/~mwhudson/launchpad/disable-test_import_bzrsvn/+merge/21984 pls
<NCommander> bac: I'm probably going to be poof soonish, and then I'm travellig, so this branch might end up sitting until next week if theres a legit test failure
<bac> NCommander: i'll let you know what happens
<NCommander> bac: thanks.
#launchpad-reviews 2010-03-24
* wgrant changed the topic of #launchpad-reviews to: on call: - || reviewing:  - || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* jelmer changed the topic of #launchpad-reviews to: on call: - || reviewing:  - || queue: [wgrant,jelmer]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* wgrant changed the topic of #launchpad-reviews to: on call: - || reviewing:  - || queue: [wgrant,jelmer,wgrant]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<NCommander> bac: any luck with EC2?
<bac> NCommander: running again now without the --headless option so i can keep an eye on it
<bac> NCommander: tried running locally last night but had failures b/c we have an issue with lucid ATM
<NCommander> bac: ugh. I feel like I sonehow managed to generate a whole lot of extra work for you
<bac> NCommander: no!  you've given us a nice branch.  i'll get it landed for you.
<leonardr> bac, want to review my fix to your bug?
<leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/shorten-cache-filename-again/+merge/22030
<bac> leonardr: sure
<bac> leonardr: your MP is private again.  how do you do keep doing that unintentionally?
<leonardr> bac: i have no idea
<leonardr> i think the branch might be private
<bac> leonardr: it doesn't bother me just thought i'd mention it
<bac> leonardr: and are you going to release this version and update LP to use it?
<bac> leonardr: looks great.  r=bac
<leonardr> bac: i'll release it. if it's really bothering you i'll do an lp update, but that's kind of a pain
<leonardr> i like to do that in batches
<bac> leonardr: i know, but without it ec2 will not run for people using ecryptfs
<leonardr> all right, i'll do a launchpad branch
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing:  - || queue: [wgrant,jelmer,wgrant]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> great
* sinzui changed the topic of #launchpad-reviews to: on call: Edwin || reviewing:  - || queue: [wgrant,jelmer,wgrant, sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> EdwinGrubbs: hi, a small one for you: https://code.edge.launchpad.net/~james-w/launchpad/fix-webservice-test-isolation/+merge/22040
<EdwinGrubbs> james_w: is it the request.publication.endRequest() that does the same thing as clear_request_started()?  I don't understand why you use different methods of cleaning up the tests.
<james_w> endRequest() calls clear_request_started() and does some other things
<james_w> In the code I added I just went for symmetry with the test setup
<EdwinGrubbs> james_w: ok, r=me
<james_w> I have no real idea what a lot of the rest of endRequest() is doing. Do you know of someone that could advise?
<EdwinGrubbs> james_w: I just noticed there are some lint errors that I would like you to clean up. I'll add them to the mp.
<EdwinGrubbs> james_w: I didn't even notice that Gary had already approved your mp. I guess I really should get some coffee.
<james_w> ah, neither did I :-)
<EdwinGrubbs> of course, it is a little confusing that the mp is still in "Needs review" status which is separate from an individual reviewer's approval.
<EdwinGrubbs> wgrant: which one of your branches would you like me to review first?
<james_w> EdwinGrubbs: would you still like me to fix the lint?
<EdwinGrubbs> james_w: don't worry about it, since it has already been reviewed.
<james_w> great, thanks
<james_w> would you land it for me please?
<EdwinGrubbs> james_w: sure
<james_w> thanks
<EdwinGrubbs> james_w: is there a bug for that branch?
<james_w> nope
<bac> NCommander: did you see the failure message?
<NCommander> bac: no
<bac> NCommander: it should've gone to your gmail acct
<NCommander> bac: wait, just got it
<bac> NCommander: your testOversizedFile had a bug in it
<bac> i can fix it and land
<NCommander> bac: How'd I break it?
<NCommander> ugh
 * NCommander thought he tested this before I submitted >.<;
<bac> NCommander: 2*20 * 10 instead of 2**20 * 10
<NCommander> bac: d'oh >.<;
<NCommander> bac: I feel like an idiot now :-/
<bac> just a typo
<NCommander> bac: so re-ec2 it?
<bac> NCommander: no, i've fixed the test and shown it to work locally. i'll just go directly to pqm with it
 * bac saves $4
<NCommander> bac: so then pqm will run the test suite, and assuming it doesn't go boom, it will land on db-devel, right?
<bac> NCommander: not quite.  pqm will accept it and later buildbot will bundle up a bunch of revisions and run them together.  they all pass or fail together and if they fail we fix and resubmit
<sinzui> EdwinGrubbs: jml: can either of you comment on bug 538024. I want to submit a patch to address this tomorrow, or I need to make this feature not be released next week.
<mup> Bug #538024: No way of saying "This project is not packaged" <Launchpad Registry:Triaged> <https://launchpad.net/bugs/538024>
<bac> NCommander: http://pastebin.ubuntu.com/400701/
<NCommander> bac: so if it fails, buildbot goes into textfix mode, right?
<bac> yes
<NCommander> bac: sweet. So how does it usually takes to run
<bac> NCommander: normally pqm is pretty quick, unless a job is in front that needs to run the entire test suite.
<NCommander> bac: pqm doesn't run the entire test suite?
<bac> no, that changed a good while ago
<NCommander> bac: didn't know that
<NCommander> bac: so how long does it take to run
<bac> NCommander: PQM?  generally about 10 minutes per branch.  your branch is 3rd in line
<NCommander> bac: is there a place to watch? :-)
<bac> https://pqm.launchpad.net/
<deryck> sinzui, could I request a *very* easy UI review from you? :-)
<sinzui> yes
<deryck> sinzui, https://code.edge.launchpad.net/~kirkland/launchpad/532624/+merge/20775
<deryck> sinzui, I just want an ack that the renaming is sane, for kirkland's branch.
<deryck> makes sense to me, but since this is from before my time and all :-)
<sinzui> deryck: They get my +1
<deryck> sinzui, excellent, thanks!
<deryck> sinzui, I requested a review from you on that MP, just to get the rubberstamp "approved"
<EdwinGrubbs> sinzui: I commented on bug 538024. I can review your brimstone-and-treacle branch now.
<mup> Bug #538024: No way of saying "This project is not packaged" <Launchpad Registry:Triaged> <https://launchpad.net/bugs/538024>
<sinzui> fab and fab
<leonardr> edwin, i have 2 very simple branches that need review
<leonardr> https://code.edge.launchpad.net/~leonardr/launchpadlib/use-1.0/+merge/22064 and https://code.edge.launchpad.net/~leonardr/launchpad/lucid-updates/+merge/22065
<NCommander> bac: sweet. my branch merged!. So, next stupid question, when will db-devel be merged into db-staging?
<bac> NCommander: some light reading: https://dev.launchpad.net/Trunk
<bac> with pretty pictures
<NCommander> bac: oops >.<;
 * NCommander hangs head
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing:  - || queue: [wgrant,jelmer,wgrant]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: on call: Edwin || reviewing:  - || queue: [wgrant,jelmer,wgrant,sinzui]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: - || reviewing:  - || queue: [wgrant,jelmer,wgrant]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> EdwinGrubbs: Sorry, I meant to be around, but no EU reviewer appeared.
<wgrant> (and there is no APAC OCR)
#launchpad-reviews 2010-03-25
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing:  wgrant || queue: [jelmer,wgrant]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi wgrant, in addition to the lucid-related failure with your soyuz-upload.txt, I also see a bunch of deprecation warnings after line 746 (printing stderror).
 * wgrant looks.
<intellectronica> noodles775: hi, i have a tiny js/ui review for you (that placement fix jtv asked for). can i add it to the queue?
<wgrant> noodles775: That *is* odd.
<noodles775> intellectronica: of course.
<wgrant> Why did that not appear before...
<noodles775> wgrant: so it wasn't there ear...
* intellectronica changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing:  wgrant || queue: [jelmer,wgrant, intellectronica]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> yeah.
<wgrant> Ah.
<wgrant> Guess which package was upgraded overnight.
<noodles775> aha.
<wgrant> I guess I could filter those warnings in ftpmaster.py
<wgrant> But it probably makes other stuff fail too.
<wgrant> It's tempting to leave that for now, rerun the test suite tonight, and fix all the introduced failures in one lot tomorrow.
<noodles775> wgrant: yeah, since I assume there will be a whole bunch of new lucid errors for soyuz tests :/
<wgrant> I think filtering those warnings in a couple of places should do it, but we'll find out.
<wgrant> That test should pass fine on Hardy still.
<noodles775> OK. I'll update the MP.
<wgrant> Yay for huge API changes in the last month of an LTS.
<noodles775> wgrant: sorry, just to check: do you want me to approve and land this branch, since the tests will pass on hardy?
<wgrant> Thanks.
<wgrant> noodles775: Please do.
<wgrant> I'll fix all the new Lucid failures tomorrow.
<noodles775> Wonderful :)
<noodles775> wgrant: with your rescueiflost/updatestatus branch, couldn't we do something like rescueIfLost = Builder.rescueIfLost on the MockBuilder if the code is the same? (and not have to define the functions?)
<wgrant> noodles775: Bound methods check the type of the first argument.
<wgrant> So no, it's not quite that easy :(
<wgrant> Er, unbound methods.
<noodles775> Ok.
<noodles775> wgrant: in which case, is there an obvious reason that I'm missing for why MockBuilder couldn't inherit from Builder and just overload all its methods as needed?
<wgrant> noodles775: It looks like a few other mocks do that. So maybe I could.
<noodles775> wgrant: OK, I'll leave it up to you - I personally think it would improve the code.
<noodles775> r=me
<wgrant> noodles775: It would, yes.
<wgrant> Thanks.
 * wgrant will fix.
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: intellectronica || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> intellectronica: where's your MP? I can't see it on active reviews?
<intellectronica> noodles775: because it's a lazr branch. https://code.edge.launchpad.net/~intellectronica/lazr-js/choiceedit-horizontal-position/+merge/22106
<noodles775> Thanks.
<wgrant> noodles775: Mmmm, because Builder inherits SQLBase it's not that simple. Simply initialising it tries to add it to the DB, and then all hell breaks loose...
<noodles775> wgrant: :/
<wgrant> So it's probably best to leave it as it is now, although it's ugly.
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: intellectronica || queue: [StevenK]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> intellectronica: I approved your MP, but please see the subsequent comment and see what you think.
<noodles775> I didn't think it was an issue at first (and hence didn't discuss it here), but it may be worth considering.
<intellectronica> noodles775: yeah, i'm not sure what's the appropriate solution (and it's very much a corner case). we could avoid doing the correction, so that at least you get to see the leftmost content
<intellectronica> then again, is it really likely that people will be using this from a screen smaller than the widget?
<intellectronica> on my phone, for example, there's enough space for the widget to display
<noodles775> intellectronica: yeah, it is a corner case, and I don't know what the average pixel width of a phone is. Given that it's not hard to avoid it, I personally think it'd be worth the extra line or two, but up to you.
<noodles775> intellectronica: that is, if you avoid the correction in that case, the user can always scroll right to see the rest. But in the current case, they can never scroll left beyond 0.
<intellectronica> noodles775: yeah, i think simply not doing the correction is the right way to go. the widget is going to be cut anyway, but at least it will be cut on the right.
* wgrant changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: intellectronica || queue: [StevenK,wgrant]  || 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: [wgrant]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> jelmer_: do you want to look at wgrant's first?
<jelmer_> noodles775: sure
<wgrant> jelmer's a mentat now?
<noodles775> Yeah.
<wgrant> Nice.
<noodles775> [, 10
<noodles775> oops, wrong kb layout ;)
<bigjools> are you a crazy dvorak dude?
 * wgrant would recognise that mistake anywhere.
* jelmer changed the topic of #launchpad-reviews to: on call: noodles775, jelmer || reviewing: wgrant || queue: [] || 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, jelmer || reviewing: wgrant,sinzui || queue: [] || 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, jelmer || reviewing: wgrant, lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> noodles775, jelmer, i need review of two tiny branches
<leonardr> https://code.edge.launchpad.net/~leonardr/launchpad/lucid-updates
<leonardr> https://code.edge.launchpad.net/~leonardr/launchpadlib/use-1.0
<leonardr> noodles775, jelmer: actually just the lucid-updates one
<leonardr> i'm going to redo that merge proposal actually to get rid of conflicts
<leonardr> noodles775: https://code.edge.launchpad.net/~leonardr/launchpad/lucid-updates/+merge/22123
* wgrant changed the topic of #launchpad-reviews to: on call: noodles775, jelmer || reviewing: wgrant, lunch || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> https://code.edge.launchpad.net/~wgrant/launchpad/sprb-package-branch-db-permission/+merge/22128 <- about as trivial as they get.
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, jelmer || reviewing: wgrant, wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> lol
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775, jelmer || reviewing: bigjools, wgrant || queue: [] || 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, jelmer || reviewing: -, wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> noodles775, still waiting for a review of https://code.edge.launchpad.net/~leonardr/launchpad/lucid-updates/+merge/22123
<noodles775> leonardr: I approved it 40mins ago?
<noodles775> s/40/31 ;)
<leonardr> ??
<leonardr> noodles775: so you did, for some reason it still says needs review
<noodles775> leonardr: yes, the status doesn't update automatically, as you might need multiple reviews (ui/code etc.) before you want to set the MP as approved.
<leonardr> ok, i guess i don't do many launchpad branches
<noodles775> leonardr: you've got the perms to update the status though right?
<leonardr> noodles775: yes
<noodles775> Great.
* wgrant changed the topic of #launchpad-reviews to: on call: noodles775, jelmer || reviewing: -, wgrant || queue: [wgrant] || 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, jelmer || reviewing: wgrant, wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> wgrant: around? With your MP, just wondering if you think it's worth adding ISPRecipe.base_branch (simply proxy to SPRData.base_branch which already exists) for use in the title?
<noodles775> Instead of including *all* the referenced branches?
<wgrant> noodles775: I considered that. I guess including all could get a bit long sometimes.
<noodles775> Yeah.
<noodles775> With that, r=me.
<noodles775> oh, I didn't send of your second branch to land yet...
* noodles775 changed the topic of #launchpad-reviews to: on call: jelmer || reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * noodles775 retires for the day to do some qa.
<wgrant> Thanks noodles775.
<wgrant> noodles775: I've fixed that branch. Do you have a moment to send it off to EC2?
<noodles775> wgrant: Sure.
<wgrant> lp:~wgrant/launchpad/sprb-package-branch-db-permission appears to be approved too -- is that in EC2 somewhere? I guess it can probably just land directly.
* jelmer changed the topic of #launchpad-reviews to: on call:|| reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* salgado changed the topic of #launchpad-reviews to: on call:|| reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-03-26
<thumper> mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/remove-thumper-from-sample-data/+merge/22189 diff still pending
<mwhudson> thumper: done
<james_w> thumper/mwhudson: could you take a look at https://code.edge.launchpad.net/~james-w/launchpad/code-imports-for-source-packages/+merge/21821 please?
<mwhudson> james_w: after lunch ok?
<james_w> sure
<mwhudson> cool
<james_w> I meant to do it as two branches, but got carried away it seems
<james_w> I can split it up if you like
<mwhudson> doesn't seem too large
<james_w> I'm going to do it, as it makes the tests a bit clearer as to what they are doing
* wgrant changed the topic of #launchpad-reviews to: on call:|| reviewing: - || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<james_w> hi mwhudson
<mwhudson> james_w: hello
<james_w> mwhudson: do you have time to look now?
<mwhudson> james_w: i guess you'd like me to look at that branch?
<mwhudson> yeah
<mwhudson> james_w: can you paste the link again?
<james_w> if not, then I'll sign off and we can do this another time
<james_w> sure
<mwhudson> resume didn't, again
<james_w> thanks
<james_w> heh
<james_w> https://code.edge.launchpad.net/~james-w/launchpad/new-code-import-method-on-branchtarget/+merge/22190
<james_w> https://code.edge.launchpad.net/~james-w/launchpad/code-imports-for-source-packages/+merge/22191
<james_w> the first is another refactoring-type branch, and then the second flips the switch and adds tests for source package imports
<mwhudson> james_w: is there an order to them?
<mwhudson> ah ok
<mwhudson> james_w: the diff for https://code.edge.launchpad.net/~james-w/launchpad/new-code-import-method-on-branchtarget/+merge/22190 has conflicts
<james_w> I'm not sure that AssertionError is the right error to be raising
<mwhudson> um
<james_w> yeah, I don't see them
<mwhudson> or at least the mp page claims it has conflicts
<mwhudson> that's a bit odd
<james_w> I think it's something to do with the prerequisite branches
<james_w> yeah, a prerequisite branch has conflicts with devel, fixing that now
<mwhudson> james_w: it all looks good, apart from a few details on whitespace of all things
<james_w> great
<mwhudson> james_w: balls, reviewed the wrong proposal
<james_w> oops
<james_w> conflicts fixed
<mwhudson> james_w: anyway, both branches reviewed
<james_w> thanks mwhudson. Both fixed as well.
<mwhudson> james_w: cool
<mwhudson> james_w: can you land these yourself yet?
<james_w> I cannot
<james_w> now I must depart
<james_w> thanks for your help
<mwhudson> bye
<wgrant> I'm guessing that there will be no OCR today, since Bugs is sprinting?
* wgrant changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [wgrant,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> wgrant, I see only one branch from you on https://code.edge.launchpad.net/launchpad-project/+activereviews
<salgado> is the topic here correct, do you have 2 branches for review?
* salgado changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue: [wgrant,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * bigjools waves at salgado
<salgado> howdy bigjools!
<bigjools> salgado: hey dude, how's it going?
<bigjools> I've got a nice branch for you :)
<salgado> you soyuz people always do. :P
<bigjools> this one's pretty easy as soyuz goes
<salgado> :)
<bigjools> popped you on the list of reviewers, you should get some email
<salgado> cool.  will get to it in a minute.  just need to submit one of my own branches to ec2
<bigjools> I should expand the MP description really
* salgado changed the topic of #launchpad-reviews to: on call: salgado || reviewing: bigjools || queue: [wgrant,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> thanks salgado
 * bigjools needs food, bbiab
* salgado changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue: [wgrant,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue: [wgrant,wgrant, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> EdwinGrubbs: can you review https://code.launchpad.net/~sinzui/launchpad/do-not-release/+merge/22239 for me. We need this landed today, or we must ask for an RC next week
<jtv> henninge: not good English: "# Not matching format specifiers will not be validated."
<jtv> "Mismatched" would work, or "non-matching."
<jtv> henninge: about "# Failing is usually propagated by an exception."âthat'd be "Failure."  And... when does it not cause an exception?
<jtv> henninge: do you know it's safe to unicode(message_or_exception) in the GettextValidationError constructor?  You might trigger an encoding error right there.  May be safer to use repr(message_or_exception) instead.
<jtv> henninge: in the validate_translation docstring, please backquote the reference to GettextValidationError
<jtv> henninge:
<jtv> 779+    elif len(translation):
<jtv> 780+        msg.set_msgstr(translation[0])
<jtv> len(translation) > 0
<jtv> and, our coding standards require an else.
<jtv> henninge: in "# Hide gettextpo.error in GettextValidationError."â"hide" doesn't sound right... how about "wrap"?
<jtv> henninge: finally, "ignore_errors" doesn't seem to mean "ignore errors," but select between exceptions and a bool return code to indicate validation failure.  Assuming this is not easy to clean up, can you come up with a better name?
<EdwinGrubbs> sinzui: sure, I'll review that now.
<bigjools> salgado: hi there
<bigjools> nearly done with the branch changes, but I'm struggling to get cancel_url and next_url working, it's completely ignoring them, any advice?
<salgado> bigjools, that's weird. is it redirecting somewhere or nowhere?
<bigjools> salgado: nowhere
<bigjools> salgado: current diff: http://pastebin.ubuntu.com/401913/
<salgado> +    @property
<salgado> +    def next_url(self):
<salgado> +        # We redirect back to the PPA owner's profile page on a
<salgado> +        # successful action.
<salgado> +        canonical_url(self.context.owner)
<salgado> bigjools, maybe if you return the value?
<salgado> ;)
<bigjools> fuck sake :)
<bigjools> it's friday ok?
<bigjools> salgado: so with those changes how's it looking?
<salgado> bigjools, you know what, I've made this same mistake a couple weeks ago and it took me quite a while to figure out what was wrong.  maybe I caught your mistake so quickly because of that
<bigjools> salgado: it's so easy to do, I really wish Python would warn if you don't return stuff from a @property
<bigjools> defaulting to None is crap
<salgado> indeed
<salgado> bigjools, you can easily teach pyflakes to do that, I think
<sinzui> salgado: how?
 * sinzui has a custome pyflakes + pep8 linter and it does not report that case
<salgado> sinzui, I might be wrong
<sinzui> pep8 could be taught though
<salgado> but I'd expect it'd be possible to hack pyflakes to check for @properties that don't return
<sinzui> well, I think it would be easier to teach pep8 that pyflakes
<salgado>          if not archive.private:
<salgado>              link.enabled = False
<salgado> -        if archive.status in (
<salgado> -            ArchiveStatus.DELETING, ArchiveStatus.DELETED):
<salgado> +        if not archive.is_active:
<salgado>              link.enabled = False
<salgado> bigjools, care to combine those two if blocks?
<bigjools> salgado: right yep
<bigjools> salgado: I have to finish now, I'll pop back later.  I'm not landing this branch this cycle anyway, I just need to be ready to land it at the start of the next so it can get some beta testing.
<salgado> bigjools, cool.  I've just approved it, btw
<bigjools> salgado: ok thanks duderino
<salgado> you're welcome
<salgado> sinzui, is the one you have queued the one that EdwinGrubbs is reviewing?
<bigjools> rockstar: do you want to take a final look at https://code.edge.launchpad.net/~julian-edwards/launchpad/ppa-deletion-ui/+merge/21925 ?
<bigjools> (ppa deletion)
<sinzui> salgado: I hope so
 * bigjools runs, goodnight all
<EdwinGrubbs> sinzui: r=me
<sinzui> EdwinGrubbs: thanks? did you have any concerns about the filtering of the obsolete/old packages?
<sinzui> EdwinGrubbs: I think I may want to adjust to sorting of Product.sourcepackages next month. The order is alphabetical. It kinds of works, but If the primary use case is listing important packages, I think I want to switch to newest first. We will have to change this eventually because we as half way through the alphabet
<EdwinGrubbs> sinzui: well, I saw that the test determined obsolete by whether there was a spph. I don't know if it would make more sense to check the distroseries status.
<sinzui> EdwinGrubbs: the series status and is the cause of the no currentrelease. We removed the release. This will fix the old packages that are listed on bzr page
<sinzui> s/and is/is/
<EdwinGrubbs> sinzui: what do you mean by "we removed the release".
<sinzui> EdwinGrubbs: I took the two properties from the branch that lets users say the project is not packaged. I can fix the sort order when I land that branch. I still am not certain the sort order is wrong. I just think the order can have surprises.
<sinzui> EdwinGrubbs: We purge the old packages when the series goes obsolete
<sinzui> EdwinGrubbs: looking at https://edge.launchpad.net/gedit, you can see that every package that is missing a currentrelease is also an obsolete series
<EdwinGrubbs> sinzui: I think you are right that sorting by date would be better than the current sorting by name.
<sinzui> right, hardy and hoary look wrong
<sinzui> That comes from the model. Since it appears to be fore display purposes, I think I will try to change that in my next branch
<gary_poster> james_w: switching here because I suppose this is the proper thing to do. :-)
<james_w> I guess :-)
<gary_poster> james_w: Did you consider AssertionError over ValueError?  I was gung ho for AssertionError, but then started convincing myself of ValueError.  I still lean towards AssertionError...ooh, no I have an idea I like better, maybe.  One sec, looking up.
<james_w> I don't really mind what the error type is
<gary_poster> OK, james_w, here is the trivia I'm struggling with.  Normally, the right thing to do in this case is to write a function (I'm going to suggest that we switch the class to a function anyway) that returns None.  That behaves properly with standard Zope calls: getMultiAdapter will raise a ComponentLookupError, and queryMultiAdapter will return None or the provided default.
<gary_poster> By explicitly raising an exception we're breaking the pattern.  Raising ComponentLookupError (what I was considering) is not appropriate here because that's the responsibility of something higher up.  And the reason we are doing this is to help the user figure out what they did wrong.  In would be nice if the zope machinery had a spelling for that, but it doesn't.
<gary_poster> So, I think an AssertionError is the right thing to do for our intentions: we're doing this to help a poor developer debug and fix their problems, which is the domain of an AssertionError.  A ValueError is an attempt to raise a "real" error, and would be the wrong "real" error (ComponentLookupError is the right error, if we get one at all).
<gary_poster> So with all that, I would suggest changing the ChoiceErrorMarshaller class to something like this:
<gary_poster> def choiceMarshallerError(field, request, vocabulary):
<gary_poster>     # We don't support marshalling a normal Choice field with a
<gary_poster>     # SQLObjectVocabularyBase-based vocabulary.
<gary_poster>     # Normally for this kind of use case, one returns None and
<gary_poster>     # lets the Zope machinery alert the user that the lookup has gone wrong.
<gary_poster>     # However, we want to be more helpful, so we make an assertion,
<gary_poster>     # with a comment on how to make things better.
<gary_poster>     assert False, (...your message...)
<james_w> ok
<gary_poster> james_w: other than that, r=gary.  You cool with that?
<james_w> yeah, let me make the changes so that you can confirm
<gary_poster> ok cool
<james_w> gary_poster: please re-check
<gary_poster> ack james_w
<gary_poster> james_w: +1.  I really would like (something like) the comment I gave to be there too.  People who know the Zope pattern will appreciate reading what's going on (like me, when I've forgotten all about this!)
<james_w> oh
<gary_poster> Other than that, that's what I had in mind.  I need to step out to the bank.  Would you like me to give you a merge-conditional approve so you can proceed without me?
<james_w> done
<james_w> I can't merge anyway, so there's no rush
<gary_poster> heh, ok
<gary_poster> you might not have pushed the branch, James, but I gave a merge-conditional
<james_w> it was just waiting for the puller, it's there now
<henninge> jtv: http://paste.ubuntu.com/401997/
* sinzui changed the topic of #launchpad-reviews to: on call: salgado || reviewing: - || queue: [wgrant,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adiroiban changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [wgrant,wgrant,adiroiban(bug-548999)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
