[00:29] <sinzui> rockstar I have a branch that needs UI review. I hope you or noodles has time to take a look at it tomorrow.
[00:30] <rockstar> sinzui, I can look at it now if you want to send it to me.
[00:42] <sinzui> rockstar, https://code.edge.launchpad.net/~sinzui/launchpad/team-participation-0/+merge/26996
[00:43] <sinzui> ^ sorry, I had a child demand my attention
[00:52] <rockstar> sinzui, looking
[01:49] <rockstar> thumper, can you do a review for me?
[01:50] <thumper> rockstar: sure
[01:50] <rockstar> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/fix-buildds/+merge/26990
[01:54] <rockstar> thumper, there are two other cherry picks that need to occur before we can re-enable recipe builds.
[01:59] <thumper> rockstar: yeah, I read your conversation with wgrant
[01:59] <rockstar> thumper, cool.
[01:59] <rockstar> thumper, I'm composing an email to the list an copying the appropriate people.
[02:00] <thumper> rockstar: done
[02:01] <rockstar> thumper, thanks.
[06:18] <rockstar> thumper, can you do a review for me?
[06:34] <rockstar> jtv, buddy!  Can you do a review for me?
[06:35] <jtv> uh-oh  :)
[06:35] <jtv> rockstar: fork it over!
[06:35] <jtv> and does this mean you're polyphasic again?
[06:36] <rockstar> jtv, no, just trying to clear off my plate of things I planned to do today.
[06:36] <rockstar> jtv, https://code.edge.launchpad.net/~rockstar/launchpad/bug-589454/+merge/27012
[06:38] <jtv> rockstar: the bug title says "creating recipe for build-from-branch oopses."  Is that what the MP is about?  At first sight they look like different bugs.
[06:38] <rockstar> jtv, yeah, they are the same bug.  See the description.
[06:39] <jtv> oic
[06:39] <rockstar> jtv, basically, LaunchpadFormView's validation has some dirty little secrets...
[06:40] <jtv> So the presence of "name" (which I suppose would be required) isn't enforced before you get to the validation method?
[06:42] <jtv> Yup, the interface marks it as required, and yet your validator may get run when it's not given.  Nasty.
[06:42] <rockstar> jtv, well, it adds the error to data.errors, but it continues on stupidly...
[06:43] <jtv> Ah, that does make sense.
[06:44] <jtv> On a coding style note, I'd prefer keeping the ISourcePackageRecipeSource utility in a variable over breaking the line all the way over on the right and continuing at what looks like a completely arbitrary offset.
[06:45] <jtv> Especially in an "if."
[06:46] <jtv> In fact, why not:
[06:47] <jtv> name = data.get('name')
[06:47] <jtv> if name is not None:
[06:47] <jtv>     recipesource = getUtility(ISourcePackageRecipeSource)
[06:47] <jtv>     if recipesource.exists(self.user, name):
[06:47] <jtv>         self.setFieldError(
[06:47] <jtv> etc.
[06:50] <jtv> rockstar: ^^
[06:51] <rockstar> jtv, sure, I can do that.
[06:51] <jtv> thanks
[06:53] <rockstar> jtv, http://pastebin.ubuntu.com/446468/
[06:53] <rockstar> And the tests still pass, so I didn't screw anything up.
[06:54] <jtv> Lovely.
[06:54] <jtv> Also, you may want to use double-quotes for free-form text strings.  Saves the hassle when an apostrophe sneaks in.
[06:55] <jtv> (e.g. if someone should ever decide that "There's" is better than "There is")
[06:57] <rockstar> jtv, that's what we have syntax highlighting for...  :)
[06:58] <jtv> Ah great, a high-tech solution for minimizing the pain of the tack you taped to your chair...
[06:59] <rockstar> jtv, I have a standing desk now, so I don't have a chair...
[06:59] <rockstar> That's how I solved that problem.
[06:59] <jtv> rockstar: Telling.  :-)  The syntax checker I use in gvim is very nice, but it throws up a lot of complaints for a thing like this.
[07:00] <rockstar> jtv, yeah, abentley turned me onto one than runs pyflakes for me as well.  I never have to run pylint anymore now.
[07:00] <jtv> I think that's the one I use.
[07:01] <jtv> Unfortunately if I were to add an apostrophe to this string, it would make me alternate between commanding vim and acknowledging error notices.
[07:02] <jtv> ^<enter>r"<enter>$<enter>r"<enter>
[07:04] <rockstar> jtv, oh, that's obnoxious...
[07:05] <jtv> rockstar: now, stop me if I'm going too far here but in the test, instead of finding all error messages on the page and indexing at [1], would it be possible to find specifically the error message for the "name" field?
[07:05] <rockstar> jtv, I think it is doing that.  Lemme look.
[07:06] <jtv> Well it's my particular thumbtack of choice I guess.  :)  I've gotten used to a style where free-form strings use double quotes (because it's nobody's business if they contain apostrophes) but short, identifier-style strings use single quotes.  I find it nice and consistent, and believe others do something similar.
[07:06] <rockstar> jtv, yeah, we don't have a style for strings specifically, but we do for docstrings.
[07:08] <jtv> When writing style rules, it's probably a good thing not to enforce things you wouldn't be willing to clean up in the existing code (both as a limit on the body of law and a cure for existing nearby code overriding style guides) but in this case I think it's common sense to use double quotes.
[07:09] <rockstar> jtv, I'll do it just this once for you, but I generally don't ever use double quotes unless I really need to.
[07:10] <rockstar> Probably because I can't be arsed to use the Shift key.
[07:10] <jtv> So... what do you do when you've got a sequence of lines of concatenated single-quoted strings and one of them gains an apostrophe?  Change all lines or just one?
[07:10] <jtv> Or avoid the apostrophe?
[07:11] <rockstar> I'll escape it if I really need.
[07:11] <jtv> ouch!  :)
[07:11] <rockstar> This probably stems from my PHP days, where a single quote was good for making sure you weren't doing something stupid in SQL.
[07:12] <jtv> For that situation I can see it making sense...  but of course if Launchpad were PHP we wouldn't be having this conversation because I wouldn't be working here.  ;-)
[07:12] <rockstar> But it's one of the niggles that I continue to use in every case except C (where single quotes and double quotes mean different things) and javascript (probably because it reminds me of C)
[07:13] <jtv> Wouldn't the rule generalize to "use double quotes when it's okay to have single quotes or apostrophes in here, or single quotes when it's not"?
[07:15] <rockstar> jtv, I think the important thing is that there is no rule.  I think every time it's been brought up, it's the equivalent of doctests v. unittests...
[07:15] <jtv> Sure.  I'm talking about your rule, not a rule in Launchpad.
[07:16] <rockstar> jtv, I don't have a rule, more than a habit.
[07:16] <rockstar> It's like crack.  I don't necessarily WANT to do it, I just do.
[07:16] <rockstar> jtv, so, "Required input is missing" is apparently the ACTUAL error message it puts next to the widget.
[07:17] <rockstar> That's about as worthless as teats on a bull...
[07:17] <jtv> rockstar: I'm not sure I follow... actual as opposed to what?
[07:18] <rockstar> jtv, like, maybe "Name is a required field" or something.
[07:19] <jtv> Ah.  Yes, could probably be better but I guess it's clear enough when it shows up right next to the input box.
 rockstar: now, stop me if I'm going too far here but in the test, instead of finding all error messages on the page and indexing at [1], would it be possible to find specifically the error message for the "name" field?
[07:19] <rockstar> I'm responding to that...
[07:19] <jtv> Ah, then I probably expressed myself poorly.
[07:20] <jtv> What I meant was: is there no more precise way to identify the error message that you're testing?
[07:21] <jtv> Right now the test finds all messages and then uses the list index [1] for this.
[07:22] <jtv> I was wondering if there's an easy way to, say, find a tag surrounding just the "name" part of the form, and then pick out the error message from that.
[07:23] <rockstar> jtv, I don't think there is a better way.
[07:23] <jtv> Then I won't waste any more of your time.  I'm approving the MP.
[07:23] <rockstar> jtv, I'm sure we can improve it, but this is the way the doctests do it as well.
[07:23] <jtv> Yes, I know and it always saddens me a bit.  Someday I guess...
[07:23] <rockstar> jtv, so, abentley and I have been trying to use unittests for testing pages.
[07:24] <jtv> It strikes me as a worthwhile idea, since pagetests have a tendency to convolute and become brittle.
[07:25] <rockstar> jtv, my favorite part is that if the page oopses, I get the actual exception instead of crap printed to my terminal.
[07:25] <jtv> Nice, yes.
[07:26] <rockstar> jtv, when the experiment is over, I'll post to the list about it.
[07:26] <jtv> Looking forward to that.
[09:56] <noodles775> Hi gmb, another one for you when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/587113-buildbase-handleStatus/+merge/27022
[09:57] <gmb> noodles775, Sure, I'll put it in the queue.
[09:57] <noodles775> Lol... too quick :) Thanks gmb.
[09:57] <gmb> Heh.
[10:11] <stub> gmb: https://code.edge.launchpad.net/~stub/launchpad/db-stats/+merge/27026 is trivial
[10:12] <stub> And as soon as I said that, spotted the bug :-P
[10:12] <gmb> stub, Thanks; I'll take a look at that before starting on sinzui's branch
[10:13] <stub> The 'per_second' like is going to get another * 60
[10:13] <stub> Which I will shortly test on production to see if it fixes the odd results I'm seeing
[10:14] <gmb> stub, r=me with that change in mind, then :)
[10:14] <stub> Ta.
[10:15] <stub> Ahh... much nicer. My daily report now has reasonable values, matching the short term reports ;)
[10:15] <gmb> :)
[10:15] <stub> All because it is a pain in the bum converting a timedelta to a number of seconds.
[10:17] <stub> https://code.edge.launchpad.net/~stub/launchpad/oauth-db/+merge/27025 is simple, but possibly security sensitive - if you don't want to review I can designate a victim familiar with OAuth.
[10:32] <gmb> stub, That sounds like a good idea.
[11:03] <gmb> noodles775, r=me
[11:03] <noodles775> Thanks gmb.
[12:33] <deryck> gmb, the small change we discussed:  https://code.edge.launchpad.net/~deryck/launchpad/ubuntu-bugs-front-timeout-590992/+merge/27036
[12:34] <deryck> ah, he's away.
[12:34] <deryck> BjornT, perhaps you could review the above MP for code and RC.  Fixes a persistent timeout on bugs.lp.net/ubuntu.
[12:35] <deryck> meant that as a question, not statement. :-)
[12:42] <leonardr> gmb, need nearly trivial review of https://code.edge.launchpad.net/~leonardr/launchpad/toggle-representation-cache/+merge/26725
[12:42] <bigjools> one more for you gmb, thanks
[12:54] <BjornT> deryck: have you confirmed that that view isn't used on other pages?
[12:56] <deryck> BjornT, the view is for every top-level bugs page, not just distro.  a grep for bug_count_items turns up nothing.  doc and browser tests pass.  I'll look at zcml to make sure nothing else uses it, though....
[13:00] <deryck> BjornT, yeah, not used except for bugs home page for product, distro, and series of those.
[13:15]  * gmb sucks at IRC again
[13:16] <bigjools> gmb: my branch is a CP candidate BTW, but that probably doesn't change much
[13:16] <gmb> bigjools, I'll take a look at leonardr's first because it's 'nearly trivial'. Also, he asked first, looking at scrollback; seems I've done him a disservice
[13:17] <bigjools> 'tis fair
[13:17] <gmb> deryck, Can I assume that BjornT's got you covered, review wise?
[13:18] <deryck> gmb, yes, he did.  Sorry I didn't update you.
[13:18] <gmb> deryck, Not to worry. Sorry I had to dash for lunch; it was that or fall over.
[13:19] <deryck> gmb, no worries :-)
[13:19] <gmb> Ah, codebrowse, your reliable suckage heartens me.
[13:20] <gmb> leonardr, r=me
[13:28] <gmb> bigjools, r=me
[14:05] <bigjools> thanks gmb
[14:35] <adeuring> gmb: could you review the mp: https://code.edge.launchpad.net/~adeuring/launchpad/expectations-bug-556499-model/+merge/27052 ?
[14:43] <leonardr> adeuring, i'll take it
[14:45] <adeuring> leonardr: thanks!
[14:52] <leonardr> adeuring, am i reviewing the whole branch or your diff against intellectronica's work?
[14:52] <adeuring> leonardr: Tom's work has already been reviewed
[14:52] <leonardr> ok
[14:52] <adeuring> so I think it is enough if you look at the diff I added to the MP
[14:57] <leonardr> adeuring: i don't know much about storm, can you explain
[14:57] <leonardr> Store.of(self.distribution).add(dsp_in_db)
[14:57] <leonardr> it looks like you're just adding a row to the distribution table?
[14:58] <adeuring> leonardr: no, Store.of(...) just returns the database, not a table.
[14:58] <leonardr> so it's whatever database the distribution is in?
[14:58] <adeuring> leonardr: yes
[14:58] <adeuring> that's a typical pattern we use to find the "right" store
[14:59] <leonardr> i don't see where you use the bug_reported_acknowledgment property.  was it used outside of that diff without being defined properly?
[14:59] <leonardr> or, is that whole thing defined inside a model class?
[14:59] <adeuring> leonardr: exactly. There is a test, let me look for the filename...
[15:00] <leonardr> doc/bug-reported-acknowledgement.txt ?
[15:00] <adeuring> leonardr: right, that's the one.
[15:00] <leonardr> ok, i'll take a quick look
[15:00] <adeuring> leonardr: and I'm working on another branch which will really sue this proerty
[15:00] <adeuring> ...property...
[15:00] <adeuring> erm, _use_ the property...
[15:07] <leonardr> adeuring, r=me
[15:07] <adeuring> leonardr: thanks!
[15:08] <leonardr> gmb, i'll take the review you've got in the queue
[15:11] <gmb> leonardr, Thanks.
[15:18] <bigjools> gmb: another CP candidate branch fer ya
[15:18] <bigjools> short one
[15:19] <gmb> bigjools, On it
[15:19] <bigjools> ta
[15:21] <leonardr> gmb, r=me with some minor changes
[15:22] <gmb> leonardr, Thanks.
[15:24] <gmb> bigjools, r=me
[15:24] <bigjools> thanks again
[15:24] <gmb> leonardr, Gah, yes, that should be targeted at devel. Never mind, I'll fix it when I pqm-land it.
[15:25] <gmb> leonardr, And apparently I can't spell.
[15:25] <leonardr> gmb, i have the same problems
[16:13]  * gmb -> afk for a short while
[18:01] <rockstar> leonardr, you're on call?
[18:07] <rockstar> bac, can you do a review for me?
[18:09] <leonardr> rockstar, yes, i'm here
[18:09] <rockstar> leonardr, can I jump in your queue?
[18:17] <leonardr> rockstar, yes, especially since there is no queue
[18:22] <rockstar> leonardr, https://code.edge.launchpad.net/~rockstar/launchpad/edit-recipe-name/+merge/27072
[18:28] <leonardr> rockstar, i assume your first validate() method is in the 'add' view?
[18:28] <rockstar> leonardr, the original validate is in a mixin, since I do share some code.
[18:29] <rockstar> leonardr, but yes, I took some code out of there, put it into the add view, and then modified it for the edit view.
[18:29] <rockstar> leonardr, oh crap, it didn't set the pre-req branch correctly.
[18:30] <rockstar> leonardr, you can ignore test_create_new_recipe_empty_name
[18:30] <leonardr> ignore it, or come back to it after you fix it?
[18:32] <rockstar> leonardr, ignore it.  It's from the pre-requisite branch that jtv reviewed last night.
[18:32] <leonardr> i see
[18:32] <rockstar> leonardr, see also: https://code.edge.launchpad.net/~rockstar/launchpad/bug-589454/+merge/27012
[18:34] <leonardr> rockstar, you removed a 'source_package = ' line from a test. it looks like it was simply unnecessray?
[18:34] <rockstar> leonardr, yeah, it was an artifact of a branch from last week.  I must have forgotten to run make lint then.
[18:35] <rockstar> leonardr, I caught it when I ran lint this time.
[18:41] <leonardr> rockstar, r=me with an additional test case
[18:41] <rockstar> leonardr, ack.  I'll get on that now.
[20:07] <mars> leonardr, ping, have some time for a very small review?: https://code.edge.launchpad.net/~mars/launchpad/re-enable-windmill-ec2-suite/+merge/27077
[20:08] <leonardr> mars, sure
[20:12] <leonardr> mars, so you've already fixed the windmill test suite, and this is just restoring the status quo where 'make check' runs correctly on ec2?
[20:12] <mars> leonardr, yes.  I ran it in parallel to reproduce what would have hung the suite, and now it passes the error just fine.
[20:13] <leonardr> ok, r=me
[20:20] <mars> leonardr, cool, thank you
[20:21] <mars> leonardr, did you mark the MP as approved?
[20:21] <leonardr> i think so., but maybe not
[20:21] <leonardr> no, i got distracted, doing that now
[21:58] <bac> leonardr: do you have time for a short MP?
[21:58] <bac> leonardr: otherwise i could ask sinzui
[21:58] <bac> https://code.edge.launchpad.net/~bac/launchpad/bug-591345/+merge/27084
[22:01] <leonardr> bac, lemme see, but sinzui might be better--i just eoded
[22:01] <leonardr> yeah, sunzui's a better choice, sorry
[22:02] <sinzui> I hope that is a malapropism for Sinzui and Sun Tze.
[22:02] <bac> i think i like sunzui better than sinzui.
[22:03] <sinzui> The characters for Sun Tze looks like Sun-ko in Japanese, making him a her
[22:03]  * sinzui is done passing useless knowledge
[22:06] <sinzui> bac: we must have 2000 lines dedicated to pmts and we have only 17 of them...and we do not want any of them
[22:09] <bac> ye
[22:09] <bac> s
[22:10] <bac> pardon me while i reboot
[22:26] <bac> sinzui: thanks for the review