[01:43] <thumper> jml: CannotUploadToPocket doesn't inherit from exception
[01:43] <jml> thumper, am I raising it?
[01:44] <jml> thumper, ahh, I'll fix the docstring
[01:44] <thumper> ta
[01:44] <jml> thumper, this code doesn't use exceptions -- I lost an argument.
[01:44]  * thumper frowns
[01:44] <thumper> with who?
[01:45] <jml> thumper, the reviewer of my previous patch in this area :)
[01:46] <thumper> # XXX: Tests
[01:49] <jml> yes
[01:50] <thumper> either fix the XXX style, or remove it
[01:50] <thumper> also julians XXX date is off by 2 years
[01:50] <jml> I could add "icle" to the line :)
[01:51] <jml> thumper, it's not off, it's moved from old code.
[01:51] <thumper> :(
[01:52] <jml> I've deleted the XXX: Tests.
[01:53] <thumper> is there a nicer way to do this? getUtility(IComponentSet)['partner']
[01:53] <jml> thumper, how do you mean "nicer"?
[01:53] <thumper> __get_item__ on a utility class seems icky
[01:54] <thumper> unclean almost
[01:54] <jml> I'm fairly sure it's the only way right now.
[01:54] <jml> I think they want to make it an enum.
[01:56] <jml> yeah, it's the only method with those semantics.
[01:56] <thumper> omg - makeSourcePackagePublishingHistory
[01:57] <jml> yeah, neat huh?
[01:58] <thumper> jml: so where does can_upload_to_archive get used for branches?
[01:59] <jml> thumper, canonical.launchpad.security.EditBranch calls it, via can_upload_linked_package(person, branch)
[02:01] <thumper> InvalidPocketForPartnerArchive docstring needs fixing
[02:02] <stub> If anyone wants staging database updates back, they can help by reviewing https://code.launchpad.net/~stub/launchpad/replication/+merge/14738
[02:02] <stub> Most of it is a SQL dump of bits of production and can be ignored.
[02:03] <stub> The rest is trivial
[02:04] <thumper> stub: done
[02:06] <jml> thumper, thanks.
[02:06] <thumper> jml: done the InvalidPocketForPartnerArchive change?
[02:06] <thumper> jml: if so r=me
[02:07] <thumper> jml: and I'll update the mp
[02:07] <jml> thumper, fixed.
[02:46] <stub> Huh. https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/14738 has approved code reviews but ec2 land doesn't agree
[02:47] <mwhudson> stub: it checks the merge proposal status
[02:47] <mwhudson> stub: is it still "needs review" ?
[02:50] <stub> nup
[02:58] <thumper> I've had that before too
[02:58] <thumper> local object cache?
[02:58] <mwhudson> replication lag?
[02:59] <mwhudson> (no)
[02:59] <stub> Only a few seconds lag - normal.
[03:00] <stub> Where does my cache live?
[03:00]  * stub finds and nukes his cache
[03:02] <stub> Now it just fails :-P
[03:02] <stub> AttributeError: 'Launchpad' object has no attribute 'credentials'
[03:07] <mwhudson> i think that's code for "the wrong version of launchpadlib"
[03:21] <stub> Still telling me its unapproved ;-P
[03:23] <thumper> stub: land manually like the old days :)
[03:25]  * thumper queues up a couple waiting hopefully
[04:45] <thumper> jml: as you were an original author, would you care to look at https://code.edge.launchpad.net/~thumper/launchpad/lp-distroseries/+merge/14777 ?
[04:46] <jml> thumper, sure.
[04:49] <thumper> jml: thanks
[04:49] <thumper> jml: I'm away making dinner though
[04:50] <thumper> jml: I'll be back later
[04:50] <jml> thumper, ok. I'll do the review on the web.
[04:53] <jml> actually, blocked now because of structured time
[12:31] <adeuring> Moin al-maisan_, could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-479331/+merge/14783 ?
[12:40] <al-maisan_> adeuring: I am very sorry, but I won't be able to review today as announced in the email I sent yesterday.
[12:40] <adeuring> al-maisan_: np, missed that mail
[12:40] <al-maisan_> adeuring: thanks for your understanding!
[17:11] <leonardr> anyone want to review a lazr.restful branch?
[17:11] <leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/latest-version/+merge/14791
[17:18] <leonardr> flacoste, maybe you want to review my branch, see if the versioning project is getting off to a good start?
[17:18] <leonardr> https://code.edge.launchpad.net/~leonardr/lazr.restful/latest-version/+merge/14791
[17:19] <flacoste> leonardr: i'm not really available to review that until next Monday
[17:19] <leonardr> ok, np, just looking for someone to look at it
[17:19] <flacoste> leonardr: i can do a post-merge review though if you find another reviewer
[18:58] <jtv> barry: https://code.edge.launchpad.net/~jtv/lazr-js/bug-480986/+merge/14801
[20:17]  * thumper looks for reviewers
[20:41] <jtv> barry: https://code.edge.launchpad.net/~jtv/lazr-js/bug-480986
[20:41] <jtv> oops, that's the bug.
[20:41] <jtv> hang on, digging up mp
[20:41] <jtv> https://code.edge.launchpad.net/~jtv/lazr-js/bug-480986/+merge/14801
[20:47] <thumper> jtv: hi
[20:47] <jtv> hey thumper
[20:47] <thumper> jtv: are you at the lazr-js thing too
[20:48]  * jtv looks around
[20:48]  * thumper is alone with no reviewers
[20:48] <jtv> yup, that's what it looks like
[20:48]  * jtv looks sympatethetic
[20:48] <jtv> okay, I mis-typed that.  What do you need?
[20:49] <thumper> jtv: three reviews :)
[20:50] <jtv> thumper: I may meet you somewhere in the middle :)
[20:50] <thumper> jtv: how about just the oops bug fix then
[20:50] <thumper> jtv: it is easy
[20:50] <jtv> gimme
[20:50] <thumper> https://code.edge.launchpad.net/~thumper/launchpad/branch-email-subject-oops/+merge/14780
[20:50] <thumper> jtv: and short
[20:50] <jtv> ok
[20:51] <jtv> it also happens to be yours
[20:52] <jtv> I thought you were asking help reviewing stuff :)
[20:52] <thumper> heh
[20:52] <thumper> no, I need reviews
[20:55] <jtv> thumper: on the _getSubject in branch.py, what you put in the docstring really belongs in a comment.
[20:56] <thumper> ok
[20:56] <thumper> jtv: how about the comment part of the docstring?
[20:56] <jtv> (And that comment would just rephrase the code, so I'd replace it with a reference to how things go differently in the other implementation)
[20:58] <jtv> thumper: yes, that'd probably do it...  but where are the invocations?  And is this method in a common base class or something?
[20:59] <thumper> yes
[20:59] <thumper> and I found the BranchMailer is a base class for BMPMailer
[20:59] <thumper> which I had to reinstate base class behaviour in
[21:00] <jtv> Then the appropriate first line for the docstring might be a "See `BaseClass`."
[21:01] <thumper> sure
[21:04] <jtv> o-kayyy.... I don't see any invocations of _getSubject in trunk either.  Is it outside the Code tree or something?
[21:05] <jtv> ah, things are falling into place now.
[21:08] <jtv> thumper: I don't think this is the right solution.
[21:09] <thumper> jtv: why
[21:09] <thumper> jtv: test proves it :)
[21:09] <thumper> jtv: lp.services.mail.basemailer
[21:09] <jtv> Painful as it is, the "subject" argument for BaseMailer is specified as a string for interpolation, not a raw subject line.
[21:10] <jtv> So if you want a "non-%-safe" string in there, seems to me the proper way of doing it is to insert that string using interpolation into the subject template.
[21:11] <thumper> jtv: actually I think you may be right :)
[21:11]  * thumper tweaks
[21:11] <jtv> well, you asked for a review, not a rubber stamp.  :-P
[21:12] <jtv> The moral of this story is "pre-imp calls are hard to come by when everyone else but your cat is on sprint."
[21:19] <rockstar> thumper, what jtv is pointing out as an issue plagues us in other places of the mailer as well.
[21:20] <rockstar> At least, I have my suspicions.
[21:36] <jtv1> rockstar: sprints?
[21:38] <rockstar> jtv, no, the string interpolation issue.
[21:39] <jtv> ohhhh, _that_
[21:43] <thumper> jtv: diff updaetd
[21:43]  * jtv looks anew
[21:48] <jtv> thumper: looks good, thanks for sticking with me
[21:49] <thumper> jtv: thanks for taking the time to review
[21:49] <jtv> I already fixed a JS bug for today, and was working on one I didn't particularly like.  :)
[21:51] <jtv> thumper: I gave it a review type of "code," so you can "ec2 land" this one
[21:52] <thumper> w00t
[21:52] <thumper> have sip working
[21:52] <thumper> jtv: thanks
[22:11] <jtv> thumper: just having a quick look at your diffoverlay branch...  your windmill test has an old-style copyright notice at the top, without the "affero" part.
[22:11] <thumper> jtv: ah
[22:12] <jtv> Sorry, not the windmill test (some distractions here), the js source
[22:12] <thumper> jtv: was copied from somewhere else
[22:12] <jtv> evidently :)
[22:12] <thumper> :)
[22:12] <thumper> will fix
[22:15] <jtv> thumper: also in the JS file, the spinner diff probably doesn't need any text besides the spinner.  The "loading..." might make more sense as an alt text for the spinner image.
[22:15] <thumper> jtv: ok
[22:17] <jtv> There's a comment "barr" in there... probably to say "ouch, I'll just have to pray for inspiration for handling this case."
[22:17] <jtv> (There's some work going on here for unifying lazr-js error handling a bit more, I believe)
[22:18] <jtv> You'll want to display the error text somewhere, and probably splash a bit of red across the page.
[22:18]  * jtv goes for coffee
[22:18] <jtv> (not far)
[22:22] <jtv> thumper: in line 64 of that diff, I think "make lint" will complain about a missing semicolon.
[22:22] <jtv> Or maybe line 63, which is where a human (as opposed to "make lint") would expect it.
[22:23] <jtv> Near the top of that file some blank lines would be nice too
[22:25] <thumper> jtv: trailing commas are not good in js object defs are they?
[22:25] <jtv> thumper: no, they're not, and IE will seize each and any instance of them as an excuse to stop working for you
[22:26] <jtv> "make lint" should catch them though
[22:26]  * thumper makes lint
[22:29] <jtv> thumper: a few notes about get_mp_class...
[22:29] <jtv> I think this is perfectly valid use of classes, but of course it's not particularly well-supported by YUI.
[22:29]  * thumper pushed lint cleanup
[22:30]  * thumper reminds himself about that method
[22:30] <jtv> It'd be useful to have a small comment above get_mp_class explaining what it does & why
[22:30] <thumper> ah
[22:30] <thumper> yes
[22:30] <jtv> Also, constructing regexes is apparently costly, so please hoist that out of the loop.
[22:31] <jtv> With a comment for the function, the comment for its final line can go afaic
[22:32] <jtv> thumper: just checked: 2 blank lines between top-level functions, same as we do in python.
[22:34] <thumper> ok
[22:34] <rockstar> thumper, so I'm looking at popupdiff, and just FYI, I'm about to put together a branch that organizes our YUI/lazr-js namespaces, because they are a super crazy.
[22:34] <thumper> rockstar: what is the personal impact of that with me?
[22:35]  * jtv bows out so rockstar and thumper can work on this one
[22:35] <thumper> jtv: thanks for the input
[22:36] <jtv> mp np
[22:36] <rockstar> thumper, it's just FYI right now, unless you take forever to land this branch, and then you're going to screw me and, I will go crazy.
[22:36] <thumper> heh
[22:36] <thumper> rockstar: btw, i have sip working with twinkle and did a conf call with kfogel and mrjazzcat and it was clear with no delay
[22:37] <rockstar> Er, screw me UP.  I need to remember to add that.
[22:37] <rockstar> thumper, great.  I'll see if I can get it configured tonight.
[22:37] <thumper> rockstar: the instructions are on the wiki
[22:38] <rockstar> thumper, you don't need a DiffOverlay.bindUI function.
[22:39] <rockstar> thumper, and you never need to make sure to call the superclass bindUI, because it's already called.
[22:39] <thumper> rockstar: I followed the instructions on the PrettyOverlay page
[22:39] <rockstar> So lines 20-25 can be deleted.
[22:39] <thumper> rockstar: it said I had to
[22:40] <rockstar> thumper, yes, they are out of date.
[22:40]  * thumper sighs
[22:40] <rockstar> thumper, the "out of date" is a known issue that we are currently fixing.
[22:40]  * thumper removes them
[22:40]  * thumper confirms it still works
[22:41] <rockstar> thumper, we should also probably remove the calls to Y.log.
[22:42] <rockstar> What's the comment on line 58?  A placeholder, or an incomplete comment.
[22:42]  * thumper frowns
[22:42] <thumper> rockstar: it does need the bindUI
[22:42] <thumper> rockstar: if I take it out it fails to work
[22:43] <thumper> rockstar: probably a placeholder originally
[22:44]  * thumper removed all the Y.log links
[22:47] <rockstar> thumper, I almost think you should wait on landing this until after yui 3.0 lands.
[22:47] <thumper> rockstar: diff has updated
[22:48] <thumper> rockstar: why?
[22:48] <thumper> rockstar: and when will that be?
[22:48] <rockstar> thumper, today if flacoste doesn't get distracted...
[22:48] <thumper> rockstar: well, then it'll have to be after yui 3
[22:48] <thumper> flacoste: don't get distracted
[22:48] <thumper> flacoste: I'd love our code to match the online docs :)
[22:49] <rockstar> thumper, in his defense, it's a 4,000 line diff.
[22:49] <thumper> rockstar: you should have been around in the early days
[22:49] <thumper> rockstar: that wasn't too unusual
[22:49] <rockstar> thumper, did you walk to school in the snow uphill both ways?
[22:49] <rockstar> :)
[22:49] <thumper> rockstar: barefoot over broken glass
[22:50] <rockstar> thumper, I don't think the diff overlay should be created at all if the diff can't be loaded.
[22:50] <thumper> rockstar: we don't know we can't load it until after we have given them some feedback that the click is doing something
[22:50] <rockstar> A popup that says "The diff can't be loaded" is probably not an ideal story.
[22:50] <thumper> we need to give some immediate feedback to the user
[22:51] <thumper> rockstar: you are right
[22:51] <thumper> it isn't ideal
[22:51] <rockstar> Well, yes, we always need to provide feedback.  A spinner is probably better.
[22:51] <thumper> I'm waiting on good error handling from you guys
[22:52] <rockstar> I just talked to Bjorn, and we were thinking you should add "spec=lazr-error-handling" in a comment around it, but then I saw how the error handling works, and it'd probably be better to use a spinner.
[22:53] <rockstar> thumper, ^^
[22:53] <thumper> rockstar: what do you mean?
[22:53] <thumper> rockstar: it has a spinner
[22:54] <thumper> rockstar: I'll add the comment though
[22:54] <deryck> rockstar, https://code.edge.launchpad.net/~deryck/lazr-js/multiline-fixed-width-412574/+merge/14808
[22:54] <deryck> rockstar, and thanks :)
[22:55] <rockstar> thumper, so, the story should be "click the diff link, get a spinner next to where you clicked (or replacing the icon), get the diff, make an overlay and populate it with the diff, show the diff"
[22:55] <thumper> rockstar: ok, there is no icon to replace
[22:56] <thumper> rockstar: and how do you show failure?
[22:56] <thumper> rockstar: perhaps when we have a good error handling story I'll agree with you
[22:58] <rockstar> thumper, yes, in this case, there is no icon to replace, but you can put a spinner in there, and then, if there's an error, you can redirect to where the diff is without javascript.
[22:58] <rockstar> Then you have a graceful degradation story as well.
[22:58] <thumper> hmm..
[22:59]  * thumper thinks for a while
[22:59] <rockstar> Our error handling story is atrocious, this we can agree on.  However, we shouldn't be making it any worse.
[23:02] <thumper> rockstar: I'll look at changing this after lunch
[23:04] <rockstar> thumper, okay.
[23:49] <thumper> rockstar: changed 
[23:49] <thumper> rockstar: pushed, and waiting for the diff to change
[23:52] <jtv> thumper: we're talking to Virtual Maris
[23:52] <rockstar> thumper, okay.  I'm not sure if I'll be able to get to it today.