=== 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
thumperjml: CannotUploadToPocket doesn't inherit from exception01:43
jmlthumper, am I raising it?01:43
jmlthumper, ahh, I'll fix the docstring01:44
jmlthumper, this code doesn't use exceptions -- I lost an argument.01:44
* thumper frowns01:44
thumperwith who?01:44
jmlthumper, the reviewer of my previous patch in this area :)01:45
thumper# XXX: Tests01:46
thumpereither fix the XXX style, or remove it01:50
thumperalso julians XXX date is off by 2 years01:50
jmlI could add "icle" to the line :)01:50
jmlthumper, it's not off, it's moved from old code.01:51
jmlI've deleted the XXX: Tests.01:52
thumperis there a nicer way to do this? getUtility(IComponentSet)['partner']01:53
jmlthumper, how do you mean "nicer"?01:53
thumper__get_item__ on a utility class seems icky01:53
thumperunclean almost01:54
jmlI'm fairly sure it's the only way right now.01:54
jmlI think they want to make it an enum.01:54
jmlyeah, it's the only method with those semantics.01:56
thumperomg - makeSourcePackagePublishingHistory01:56
jmlyeah, neat huh?01:57
thumperjml: so where does can_upload_to_archive get used for branches?01:58
jmlthumper, canonical.launchpad.security.EditBranch calls it, via can_upload_linked_package(person, branch)01:59
thumperInvalidPocketForPartnerArchive docstring needs fixing02:01
stubIf anyone wants staging database updates back, they can help by reviewing https://code.launchpad.net/~stub/launchpad/replication/+merge/1473802:02
stubMost of it is a SQL dump of bits of production and can be ignored.02:02
stubThe rest is trivial02:03
thumperstub: done02:04
jmlthumper, thanks.02:06
thumperjml: done the InvalidPocketForPartnerArchive change?02:06
thumperjml: if so r=me02:06
thumperjml: and I'll update the mp02:07
jmlthumper, fixed.02:07
stubHuh. https://code.edge.launchpad.net/~stub/launchpad/replication/+merge/14738 has approved code reviews but ec2 land doesn't agree02:46
mwhudsonstub: it checks the merge proposal status02:47
mwhudsonstub: is it still "needs review" ?02:47
thumperI've had that before too02:58
thumperlocal object cache?02:58
mwhudsonreplication lag?02:58
stubOnly a few seconds lag - normal.02:59
stubWhere does my cache live?03:00
* stub finds and nukes his cache03:00
stubNow it just fails :-P03:02
stubAttributeError: 'Launchpad' object has no attribute 'credentials'03:02
mwhudsoni think that's code for "the wrong version of launchpadlib"03:07
stubStill telling me its unapproved ;-P03:21
thumperstub: 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 hopefully03: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
thumperjml: 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
jmlthumper, sure.04:46
thumperjml: thanks04:49
thumperjml: I'm away making dinner though04:49
thumperjml: I'll be back later04:50
jmlthumper, ok. I'll do the review on the web.04:50
jmlactually, blocked now because of structured time04: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
adeuringMoin 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
adeuringal-maisan_: np, missed that mail12: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
leonardranyone want to review a lazr.restful branch?17:11
leonardrflacoste, maybe you want to review my branch, see if the versioning project is getting off to a good start?17:18
flacosteleonardr: i'm not really available to review that until next Monday17:19
leonardrok, np, just looking for someone to look at it17:19
flacosteleonardr: i can do a post-merge review though if you find another reviewer17:19
=== danilos is now known as danilo-afk
jtvbarry: https://code.edge.launchpad.net/~jtv/lazr-js/bug-480986/+merge/1480118:58
=== deryck is now known as deryck[lunch]
* thumper looks for reviewers20:17
jtvbarry: https://code.edge.launchpad.net/~jtv/lazr-js/bug-48098620:41
jtvoops, that's the bug.20:41
jtvhang on, digging up mp20:41
=== matsubara is now known as matsubara-afk
thumperjtv: hi20:47
jtvhey thumper20:47
thumperjtv: are you at the lazr-js thing too20:47
* jtv looks around20:48
* thumper is alone with no reviewers20:48
jtvyup, that's what it looks like20:48
* jtv looks sympatethetic20:48
jtvokay, I mis-typed that.  What do you need?20:48
thumperjtv: three reviews :)20:49
jtvthumper: I may meet you somewhere in the middle :)20:50
thumperjtv: how about just the oops bug fix then20:50
thumperjtv: it is easy20:50
thumperjtv: and short20:50
jtvit also happens to be yours20:51
jtvI thought you were asking help reviewing stuff :)20:52
thumperno, I need reviews20:52
jtvthumper: on the _getSubject in branch.py, what you put in the docstring really belongs in a comment.20:55
thumperjtv: 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
jtvthumper: yes, that'd probably do it...  but where are the invocations?  And is this method in a common base class or something?20:58
thumperand I found the BranchMailer is a base class for BMPMailer20:59
thumperwhich I had to reinstate base class behaviour in20:59
jtvThen the appropriate first line for the docstring might be a "See `BaseClass`."21:00
jtvo-kayyy.... I don't see any invocations of _getSubject in trunk either.  Is it outside the Code tree or something?21:04
jtvah, things are falling into place now.21:05
jtvthumper: I don't think this is the right solution.21:08
thumperjtv: why21:09
thumperjtv: test proves it :)21:09
thumperjtv: lp.services.mail.basemailer21:09
jtvPainful as it is, the "subject" argument for BaseMailer is specified as a string for interpolation, not a raw subject line.21:09
jtvSo 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
thumperjtv: actually I think you may be right :)21:11
* thumper tweaks21:11
jtvwell, you asked for a review, not a rubber stamp.  :-P21:11
jtvThe moral of this story is "pre-imp calls are hard to come by when everyone else but your cat is on sprint."21:12
rockstarthumper, what jtv is pointing out as an issue plagues us in other places of the mailer as well.21:19
rockstarAt least, I have my suspicions.21:20
jtv1rockstar: sprints?21:36
=== jtv1 is now known as jtv
rockstarjtv, no, the string interpolation issue.21:38
jtvohhhh, _that_21:39
thumperjtv: diff updaetd21:43
* jtv looks anew21:43
jtvthumper: looks good, thanks for sticking with me21:48
thumperjtv: thanks for taking the time to review21:49
jtvI 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
jtvthumper: I gave it a review type of "code," so you can "ec2 land" this one21:51
thumperhave sip working21:52
thumperjtv: thanks21:52
jtvthumper: 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
thumperjtv: ah22:11
jtvSorry, not the windmill test (some distractions here), the js source22:12
thumperjtv: was copied from somewhere else22:12
jtvevidently :)22:12
thumperwill fix22:12
jtvthumper: 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
thumperjtv: ok22:15
jtvThere'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
jtvYou'll want to display the error text somewhere, and probably splash a bit of red across the page.22:18
* jtv goes for coffee22:18
jtv(not far)22:18
jtvthumper: in line 64 of that diff, I think "make lint" will complain about a missing semicolon.22:22
jtvOr maybe line 63, which is where a human (as opposed to "make lint") would expect it.22:22
jtvNear the top of that file some blank lines would be nice too22:23
thumperjtv: trailing commas are not good in js object defs are they?22:25
jtvthumper: no, they're not, and IE will seize each and any instance of them as an excuse to stop working for you22:25
jtv"make lint" should catch them though22:26
* thumper makes lint22:26
jtvthumper: a few notes about get_mp_class...22:29
jtvI think this is perfectly valid use of classes, but of course it's not particularly well-supported by YUI.22:29
* thumper pushed lint cleanup22:29
* thumper reminds himself about that method22:30
jtvIt'd be useful to have a small comment above get_mp_class explaining what it does & why22:30
jtvAlso, constructing regexes is apparently costly, so please hoist that out of the loop.22:30
jtvWith a comment for the function, the comment for its final line can go afaic22:31
jtvthumper: just checked: 2 blank lines between top-level functions, same as we do in python.22:32
rockstarthumper, 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
thumperrockstar: what is the personal impact of that with me?22:34
* jtv bows out so rockstar and thumper can work on this one22:35
thumperjtv: thanks for the input22:35
jtvmp np22:36
rockstarthumper, 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
thumperrockstar: btw, i have sip working with twinkle and did a conf call with kfogel and mrjazzcat and it was clear with no delay22:36
rockstarEr, screw me UP.  I need to remember to add that.22:37
rockstarthumper, great.  I'll see if I can get it configured tonight.22:37
thumperrockstar: the instructions are on the wiki22:37
rockstarthumper, you don't need a DiffOverlay.bindUI function.22:38
rockstarthumper, and you never need to make sure to call the superclass bindUI, because it's already called.22:39
thumperrockstar: I followed the instructions on the PrettyOverlay page22:39
rockstarSo lines 20-25 can be deleted.22:39
thumperrockstar: it said I had to22:39
rockstarthumper, yes, they are out of date.22:40
* thumper sighs22:40
rockstarthumper, the "out of date" is a known issue that we are currently fixing.22:40
* thumper removes them22:40
* thumper confirms it still works22:40
rockstarthumper, we should also probably remove the calls to Y.log.22:41
rockstarWhat's the comment on line 58?  A placeholder, or an incomplete comment.22:42
* thumper frowns22:42
thumperrockstar: it does need the bindUI22:42
thumperrockstar: if I take it out it fails to work22:42
thumperrockstar: probably a placeholder originally22:43
* thumper removed all the Y.log links22:44
rockstarthumper, I almost think you should wait on landing this until after yui 3.0 lands.22:47
thumperrockstar: diff has updated22:47
thumperrockstar: why?22:48
thumperrockstar: and when will that be?22:48
rockstarthumper, today if flacoste doesn't get distracted...22:48
thumperrockstar: well, then it'll have to be after yui 322:48
thumperflacoste: don't get distracted22:48
thumperflacoste: I'd love our code to match the online docs :)22:48
rockstarthumper, in his defense, it's a 4,000 line diff.22:49
thumperrockstar: you should have been around in the early days22:49
thumperrockstar: that wasn't too unusual22:49
rockstarthumper, did you walk to school in the snow uphill both ways?22:49
thumperrockstar: barefoot over broken glass22:49
rockstarthumper, I don't think the diff overlay should be created at all if the diff can't be loaded.22:50
thumperrockstar: we don't know we can't load it until after we have given them some feedback that the click is doing something22:50
rockstarA popup that says "The diff can't be loaded" is probably not an ideal story.22:50
thumperwe need to give some immediate feedback to the user22:50
thumperrockstar: you are right22:51
thumperit isn't ideal22:51
rockstarWell, yes, we always need to provide feedback.  A spinner is probably better.22:51
thumperI'm waiting on good error handling from you guys22:51
=== abentley1 is now known as abentley
rockstarI 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
rockstarthumper, ^^22:53
thumperrockstar: what do you mean?22:53
thumperrockstar: it has a spinner22:53
thumperrockstar: I'll add the comment though22:54
deryckrockstar, https://code.edge.launchpad.net/~deryck/lazr-js/multiline-fixed-width-412574/+merge/1480822:54
deryckrockstar, and thanks :)22:54
rockstarthumper, 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
thumperrockstar: ok, there is no icon to replace22:55
thumperrockstar: and how do you show failure?22:56
thumperrockstar: perhaps when we have a good error handling story I'll agree with you22:56
rockstarthumper, 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
rockstarThen you have a graceful degradation story as well.22:58
* thumper thinks for a while22:59
rockstarOur error handling story is atrocious, this we can agree on.  However, we shouldn't be making it any worse.22:59
thumperrockstar: I'll look at changing this after lunch23:02
rockstarthumper, okay.23:04
thumperrockstar: changed 23:49
thumperrockstar: pushed, and waiting for the diff to change23:49
jtvthumper: we're talking to Virtual Maris23:52
rockstarthumper, 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!