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