| === 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 | ||
| thumper | jml: CannotUploadToPocket doesn't inherit from exception | 01:43 |
|---|---|---|
| jml | thumper, am I raising it? | 01:43 |
| 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:44 |
| jml | thumper, the reviewer of my previous patch in this area :) | 01:45 |
| thumper | # XXX: Tests | 01:46 |
| jml | yes | 01:49 |
| 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:50 |
| jml | thumper, it's not off, it's moved from old code. | 01:51 |
| thumper | :( | 01:51 |
| jml | I've deleted the XXX: Tests. | 01:52 |
| 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:53 |
| 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:54 |
| jml | yeah, it's the only method with those semantics. | 01:56 |
| thumper | omg - makeSourcePackagePublishingHistory | 01:56 |
| jml | yeah, neat huh? | 01:57 |
| thumper | jml: so where does can_upload_to_archive get used for branches? | 01:58 |
| jml | thumper, canonical.launchpad.security.EditBranch calls it, via can_upload_linked_package(person, branch) | 01:59 |
| thumper | InvalidPocketForPartnerArchive docstring needs fixing | 02:01 |
| 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:02 |
| stub | The rest is trivial | 02:03 |
| thumper | stub: done | 02:04 |
| jml | thumper, thanks. | 02:06 |
| thumper | jml: done the InvalidPocketForPartnerArchive change? | 02:06 |
| thumper | jml: if so r=me | 02:06 |
| thumper | jml: and I'll update the mp | 02:07 |
| jml | thumper, fixed. | 02:07 |
| stub | Huh. https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/14738 has approved code reviews but ec2 land doesn't agree | 02:46 |
| mwhudson | stub: it checks the merge proposal status | 02:47 |
| mwhudson | stub: is it still "needs review" ? | 02:47 |
| stub | nup | 02:50 |
| thumper | I've had that before too | 02:58 |
| thumper | local object cache? | 02:58 |
| mwhudson | replication lag? | 02:58 |
| mwhudson | (no) | 02:59 |
| stub | Only a few seconds lag - normal. | 02:59 |
| stub | Where does my cache live? | 03:00 |
| * stub finds and nukes his cache | 03:00 | |
| stub | Now it just fails :-P | 03:02 |
| stub | AttributeError: 'Launchpad' object has no attribute 'credentials' | 03:02 |
| mwhudson | i think that's code for "the wrong version of launchpadlib" | 03:07 |
| stub | Still telling me its unapproved ;-P | 03:21 |
| thumper | stub: land manually like the old days :) | 03:23 |
| === 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 | ||
| * thumper queues up a couple waiting hopefully | 03:25 | |
| === 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 | ||
| 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:45 |
| jml | thumper, sure. | 04:46 |
| thumper | jml: thanks | 04:49 |
| thumper | jml: I'm away making dinner though | 04:49 |
| thumper | jml: I'll be back later | 04:50 |
| jml | thumper, ok. I'll do the review on the web. | 04:50 |
| jml | actually, blocked now because of structured time | 04:53 |
| === 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 | ||
| adeuring | Moin al-maisan_, could you please review this mp: https://code.edge.launchpad.net/~adeuring/launchpad/bug-479331/+merge/14783 ? | 12:31 |
| === mrevell is now known as mrevell-lunch | ||
| 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! | 12:40 |
| === 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 | ||
| 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:11 |
| 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:18 |
| 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 | 17:19 |
| === danilos is now known as danilo-afk | ||
| jtv | barry: https://code.edge.launchpad.net/~jtv/lazr-js/bug-480986/+merge/14801 | 18:58 |
| === deryck is now known as deryck[lunch] | ||
| * thumper looks for reviewers | 20:17 | |
| 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:41 |
| === matsubara is now known as matsubara-afk | ||
| thumper | jtv: hi | 20:47 |
| jtv | hey thumper | 20:47 |
| thumper | jtv: are you at the lazr-js thing too | 20:47 |
| * 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:48 |
| thumper | jtv: three reviews :) | 20:49 |
| 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:50 |
| jtv | it also happens to be yours | 20:51 |
| jtv | I thought you were asking help reviewing stuff :) | 20:52 |
| thumper | heh | 20:52 |
| thumper | no, I need reviews | 20:52 |
| jtv | thumper: on the _getSubject in branch.py, what you put in the docstring really belongs in a comment. | 20:55 |
| 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:56 |
| 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:58 |
| 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 | 20:59 |
| jtv | Then the appropriate first line for the docstring might be a "See `BaseClass`." | 21:00 |
| thumper | sure | 21:01 |
| jtv | o-kayyy.... I don't see any invocations of _getSubject in trunk either. Is it outside the Code tree or something? | 21:04 |
| jtv | ah, things are falling into place now. | 21:05 |
| jtv | thumper: I don't think this is the right solution. | 21:08 |
| 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:09 |
| 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:10 |
| 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:11 |
| 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:12 |
| rockstar | thumper, what jtv is pointing out as an issue plagues us in other places of the mailer as well. | 21:19 |
| rockstar | At least, I have my suspicions. | 21:20 |
| jtv1 | rockstar: sprints? | 21:36 |
| === jtv1 is now known as jtv | ||
| rockstar | jtv, no, the string interpolation issue. | 21:38 |
| jtv | ohhhh, _that_ | 21:39 |
| thumper | jtv: diff updaetd | 21:43 |
| * jtv looks anew | 21:43 | |
| jtv | thumper: looks good, thanks for sticking with me | 21:48 |
| 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:49 |
| === abentley1 is now known as abentley | ||
| jtv | thumper: I gave it a review type of "code," so you can "ec2 land" this one | 21:51 |
| thumper | w00t | 21:52 |
| thumper | have sip working | 21:52 |
| thumper | jtv: thanks | 21:52 |
| 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:11 |
| 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:12 |
| 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:15 |
| 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:17 |
| 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:18 |
| 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:22 |
| jtv | Near the top of that file some blank lines would be nice too | 22:23 |
| 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:25 |
| jtv | "make lint" should catch them though | 22:26 |
| * thumper makes lint | 22:26 | |
| 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:29 | |
| * 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:30 |
| jtv | With a comment for the function, the comment for its final line can go afaic | 22:31 |
| jtv | thumper: just checked: 2 blank lines between top-level functions, same as we do in python. | 22:32 |
| 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:34 |
| * jtv bows out so rockstar and thumper can work on this one | 22:35 | |
| thumper | jtv: thanks for the input | 22:35 |
| 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:36 |
| 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:37 |
| rockstar | thumper, you don't need a DiffOverlay.bindUI function. | 22:38 |
| 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:39 |
| 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:40 | |
| rockstar | thumper, we should also probably remove the calls to Y.log. | 22:41 |
| 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:42 |
| thumper | rockstar: probably a placeholder originally | 22:43 |
| * thumper removed all the Y.log links | 22:44 | |
| 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:47 |
| 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:48 |
| 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:49 |
| 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:50 |
| 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:51 |
| === abentley1 is now known as abentley | ||
| 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:52 |
| rockstar | thumper, ^^ | 22:53 |
| thumper | rockstar: what do you mean? | 22:53 |
| thumper | rockstar: it has a spinner | 22:53 |
| 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:54 |
| 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:55 |
| 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:56 |
| 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:58 |
| * 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. | 22:59 |
| thumper | rockstar: I'll look at changing this after lunch | 23:02 |
| rockstar | thumper, okay. | 23:04 |
| thumper | rockstar: changed | 23:49 |
| thumper | rockstar: pushed, and waiting for the diff to change | 23:49 |
| 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. | 23:52 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!