=== matsubara-afk is now known as matsubara === sinzui changed the topic of #launchpad-reviews to: On Call: - || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [00:29] 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] sinzui, I can look at it now if you want to send it to me. [00:42] rockstar, https://code.edge.launchpad.net/~sinzui/launchpad/team-participation-0/+merge/26996 [00:43] ^ sorry, I had a child demand my attention [00:52] sinzui, looking [01:49] thumper, can you do a review for me? [01:50] rockstar: sure [01:50] thumper, https://code.edge.launchpad.net/~rockstar/launchpad/fix-buildds/+merge/26990 [01:54] thumper, there are two other cherry picks that need to occur before we can re-enable recipe builds. [01:59] rockstar: yeah, I read your conversation with wgrant [01:59] thumper, cool. [01:59] thumper, I'm composing an email to the list an copying the appropriate people. [02:00] rockstar: done [02:01] thumper, thanks. === matsubara is now known as matsubara-afk === ajmitch_ is now known as ajmitch [06:18] thumper, can you do a review for me? [06:34] jtv, buddy! Can you do a review for me? [06:35] uh-oh :) [06:35] rockstar: fork it over! [06:35] and does this mean you're polyphasic again? [06:36] jtv, no, just trying to clear off my plate of things I planned to do today. [06:36] jtv, https://code.edge.launchpad.net/~rockstar/launchpad/bug-589454/+merge/27012 [06:38] 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] jtv, yeah, they are the same bug. See the description. [06:39] oic [06:39] jtv, basically, LaunchpadFormView's validation has some dirty little secrets... [06:40] So the presence of "name" (which I suppose would be required) isn't enforced before you get to the validation method? [06:42] Yup, the interface marks it as required, and yet your validator may get run when it's not given. Nasty. [06:42] jtv, well, it adds the error to data.errors, but it continues on stupidly... [06:43] Ah, that does make sense. [06:44] 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] Especially in an "if." [06:46] In fact, why not: [06:47] name = data.get('name') [06:47] if name is not None: [06:47] recipesource = getUtility(ISourcePackageRecipeSource) [06:47] if recipesource.exists(self.user, name): [06:47] self.setFieldError( [06:47] etc. [06:50] rockstar: ^^ [06:51] jtv, sure, I can do that. [06:51] thanks [06:53] jtv, http://pastebin.ubuntu.com/446468/ [06:53] And the tests still pass, so I didn't screw anything up. [06:54] Lovely. [06:54] Also, you may want to use double-quotes for free-form text strings. Saves the hassle when an apostrophe sneaks in. [06:55] (e.g. if someone should ever decide that "There's" is better than "There is") [06:57] jtv, that's what we have syntax highlighting for... :) [06:58] Ah great, a high-tech solution for minimizing the pain of the tack you taped to your chair... [06:59] jtv, I have a standing desk now, so I don't have a chair... [06:59] That's how I solved that problem. [06:59] 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] jtv, yeah, abentley turned me onto one than runs pyflakes for me as well. I never have to run pylint anymore now. [07:00] I think that's the one I use. [07:01] 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] ^r"$r" [07:04] jtv, oh, that's obnoxious... [07:05] 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] jtv, I think it is doing that. Lemme look. [07:06] 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] jtv, yeah, we don't have a style for strings specifically, but we do for docstrings. [07:08] 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] 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] Probably because I can't be arsed to use the Shift key. [07:10] 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] Or avoid the apostrophe? [07:11] I'll escape it if I really need. [07:11] ouch! :) [07:11] 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] 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] 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] 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] 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] Sure. I'm talking about your rule, not a rule in Launchpad. [07:16] jtv, I don't have a rule, more than a habit. [07:16] It's like crack. I don't necessarily WANT to do it, I just do. [07:16] jtv, so, "Required input is missing" is apparently the ACTUAL error message it puts next to the widget. [07:17] That's about as worthless as teats on a bull... [07:17] rockstar: I'm not sure I follow... actual as opposed to what? [07:18] jtv, like, maybe "Name is a required field" or something. [07:19] Ah. Yes, could probably be better but I guess it's clear enough when it shows up right next to the input box. [07:19] 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] I'm responding to that... [07:19] Ah, then I probably expressed myself poorly. [07:20] What I meant was: is there no more precise way to identify the error message that you're testing? [07:21] Right now the test finds all messages and then uses the list index [1] for this. [07:22] 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] jtv, I don't think there is a better way. [07:23] Then I won't waste any more of your time. I'm approving the MP. [07:23] jtv, I'm sure we can improve it, but this is the way the doctests do it as well. [07:23] Yes, I know and it always saddens me a bit. Someday I guess... [07:23] jtv, so, abentley and I have been trying to use unittests for testing pages. [07:24] It strikes me as a worthwhile idea, since pagetests have a tendency to convolute and become brittle. [07:25] jtv, my favorite part is that if the page oopses, I get the actual exception instead of crap printed to my terminal. [07:25] Nice, yes. [07:26] jtv, when the experiment is over, I'll post to the list about it. [07:26] Looking forward to that. === gary_poster_ is now known as gary_poster === mwhudson_ is now known as mwhudson === gmb changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === gmb changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [09:56] Hi gmb, another one for you when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/587113-buildbase-handleStatus/+merge/27022 === noodles775 changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: - || queue: [sinzui, noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [09:57] noodles775, Sure, I'll put it in the queue. === gmb changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: - || queue: [sinzui, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [09:57] Lol... too quick :) Thanks gmb. [09:57] Heh. [10:11] gmb: https://code.edge.launchpad.net/~stub/launchpad/db-stats/+merge/27026 is trivial [10:12] And as soon as I said that, spotted the bug :-P [10:12] stub, Thanks; I'll take a look at that before starting on sinzui's branch [10:13] The 'per_second' like is going to get another * 60 [10:13] Which I will shortly test on production to see if it fixes the odd results I'm seeing [10:14] stub, r=me with that change in mind, then :) [10:14] Ta. === gmb changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: sinzui || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [10:15] Ahh... much nicer. My daily report now has reasonable values, matching the short term reports ;) [10:15] :) [10:15] All because it is a pain in the bum converting a timedelta to a number of seconds. [10:17] 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] stub, That sounds like a good idea. === gmb changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [11:03] noodles775, r=me === gmb changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [11:03] Thanks gmb. === gmb changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [12:33] gmb, the small change we discussed: https://code.edge.launchpad.net/~deryck/launchpad/ubuntu-bugs-front-timeout-590992/+merge/27036 === deryck changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: lunch || queue: [deryck] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [12:34] ah, he's away. [12:34] BjornT, perhaps you could review the above MP for code and RC. Fixes a persistent timeout on bugs.lp.net/ubuntu. [12:35] meant that as a question, not statement. :-) [12:42] gmb, need nearly trivial review of https://code.edge.launchpad.net/~leonardr/launchpad/toggle-representation-cache/+merge/26725 === bigjools changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: lunch || queue: [deryck, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [12:42] one more for you gmb, thanks [12:54] deryck: have you confirmed that that view isn't used on other pages? [12:56] 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] BjornT, yeah, not used except for bugs home page for product, distro, and series of those. === Ursinha-afk is now known as Ursinha === gmb changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === gmb changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: - || queue: [deryck, bigjools, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [13:15] * gmb sucks at IRC again [13:16] gmb: my branch is a CP candidate BTW, but that probably doesn't change much [13:16] 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 === gmb changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: leonardr || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [13:17] 'tis fair [13:17] deryck, Can I assume that BjornT's got you covered, review wise? [13:18] gmb, yes, he did. Sorry I didn't update you. [13:18] deryck, Not to worry. Sorry I had to dash for lunch; it was that or fall over. [13:19] gmb, no worries :-) [13:19] Ah, codebrowse, your reliable suckage heartens me. [13:20] leonardr, r=me === gmb changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [13:28] bigjools, r=me === gmb changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [14:05] thanks gmb === gmb changed the topic of #launchpad-reviews to: On Call: gmb || reviewing: - || queue: [gmb(http://bit.ly/cy3QjX)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [14:35] gmb: could you review the mp: https://code.edge.launchpad.net/~adeuring/launchpad/expectations-bug-556499-model/+merge/27052 ? === leonardr changed the topic of #launchpad-reviews to: On Call: gmb, leonardr || reviewing: - || queue: [gmb(http://bit.ly/cy3QjX)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [14:43] adeuring, i'll take it [14:45] leonardr: thanks! [14:52] adeuring, am i reviewing the whole branch or your diff against intellectronica's work? [14:52] leonardr: Tom's work has already been reviewed [14:52] ok [14:52] so I think it is enough if you look at the diff I added to the MP [14:57] adeuring: i don't know much about storm, can you explain [14:57] Store.of(self.distribution).add(dsp_in_db) [14:57] it looks like you're just adding a row to the distribution table? [14:58] leonardr: no, Store.of(...) just returns the database, not a table. [14:58] so it's whatever database the distribution is in? [14:58] leonardr: yes [14:58] that's a typical pattern we use to find the "right" store [14:59] 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] or, is that whole thing defined inside a model class? [14:59] leonardr: exactly. There is a test, let me look for the filename... [15:00] doc/bug-reported-acknowledgement.txt ? [15:00] leonardr: right, that's the one. [15:00] ok, i'll take a quick look [15:00] leonardr: and I'm working on another branch which will really sue this proerty [15:00] ...property... [15:00] erm, _use_ the property... [15:07] adeuring, r=me [15:07] leonardr: thanks! [15:08] gmb, i'll take the review you've got in the queue [15:11] leonardr, Thanks. === bigjools changed the topic of #launchpad-reviews to: On Call: gmb, leonardr || reviewing: - || queue: [gmb(http://bit.ly/cy3QjX), bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [15:18] gmb: another CP candidate branch fer ya [15:18] short one [15:19] bigjools, On it [15:19] ta === gmb changed the topic of #launchpad-reviews to: On Call: gmb, leonardr || reviewing: bigjools, gmb || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [15:21] gmb, r=me with some minor changes [15:22] leonardr, Thanks. [15:24] bigjools, r=me [15:24] thanks again [15:24] leonardr, Gah, yes, that should be targeted at devel. Never mind, I'll fix it when I pqm-land it. [15:25] leonardr, And apparently I can't spell. [15:25] gmb, i have the same problems === leonardr is now known as leonardr-afk === gmb changed the topic of #launchpad-reviews to: On Call: gmb, leonardr || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [16:13] * gmb -> afk for a short while === leonardr-afk is now known as leonardr === Ursinha is now known as Ursinha-lunch === gmb changed the topic of #launchpad-reviews to: On Call: leonardr || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === Ursinha-lunch is now known as Ursinha [18:01] leonardr, you're on call? [18:07] bac, can you do a review for me? [18:09] rockstar, yes, i'm here [18:09] leonardr, can I jump in your queue? [18:17] rockstar, yes, especially since there is no queue === leonardr changed the topic of #launchpad-reviews to: On Call: leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [18:22] leonardr, https://code.edge.launchpad.net/~rockstar/launchpad/edit-recipe-name/+merge/27072 [18:28] rockstar, i assume your first validate() method is in the 'add' view? [18:28] leonardr, the original validate is in a mixin, since I do share some code. [18:29] 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] leonardr, oh crap, it didn't set the pre-req branch correctly. [18:30] leonardr, you can ignore test_create_new_recipe_empty_name [18:30] ignore it, or come back to it after you fix it? [18:32] leonardr, ignore it. It's from the pre-requisite branch that jtv reviewed last night. [18:32] i see [18:32] leonardr, see also: https://code.edge.launchpad.net/~rockstar/launchpad/bug-589454/+merge/27012 [18:34] rockstar, you removed a 'source_package = ' line from a test. it looks like it was simply unnecessray? [18:34] leonardr, yeah, it was an artifact of a branch from last week. I must have forgotten to run make lint then. [18:35] leonardr, I caught it when I ran lint this time. [18:41] rockstar, r=me with an additional test case [18:41] leonardr, ack. I'll get on that now. === deryck is now known as deryck[lunch] === deryck[lunch] is now known as deryck [20:07] 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] mars, sure [20:12] 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] 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] ok, r=me [20:20] leonardr, cool, thank you [20:21] leonardr, did you mark the MP as approved? [20:21] i think so., but maybe not [20:21] no, i got distracted, doing that now [21:58] leonardr: do you have time for a short MP? [21:58] leonardr: otherwise i could ask sinzui [21:58] https://code.edge.launchpad.net/~bac/launchpad/bug-591345/+merge/27084 [22:01] bac, lemme see, but sinzui might be better--i just eoded [22:01] yeah, sunzui's a better choice, sorry === leonardr 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 [22:02] I hope that is a malapropism for Sinzui and Sun Tze. [22:02] i think i like sunzui better than sinzui. [22:03] 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] 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] ye [22:09] s [22:10] pardon me while i reboot [22:26] sinzui: thanks for the review