[09:08] Very strict reviewer wanted for 1-line change! https://code.launchpad.net/~jtv/launchpad/bug-515702/+merge/18411 [09:22] * al-maisan is not very strict but looks nevertheless :) [09:24] jtv: r=me [09:25] al-maisan: thanks! I'll get back to your review in a bit... But a note: you moved the construction of the "default value" for _getHeadJobPlatform into the method, but you're not using the value anywhere. [09:25] * al-maisan looks [09:27] jtv: you're right, I'll revise the code to return the (currently unused) default if there is no head job [09:28] thanks [09:28] I have https://code.edge.launchpad.net/~jpds/launchpad/fix_515388/+merge/18413 if anyone has time. :) [09:30] ...tense silence... [09:30] :-) [09:30] al-maisan: I'd feel a lot better if test_job_delay_for_unspecified_virtualization didn't rely on sample data, which it seems to do. Wouldn't it be easier to have a test helper that clears out the existing jobs, so you can make a fresh start? [09:30] al-maisan: or is that data set up by setUp? [09:31] jtv: the latter [09:31] oic [09:31] the tests in test_buildqueue.py do not use any sample data [09:32] right ho [09:35] still, the test tends towards heavy data sets... for estimation in particular, is there no way you can unit-test against smaller queues and explore the tricky cases one by one? [09:35] Because if it works for 3 queue items, chances are it's going to work for these 18 as well. [09:37] When you put the diversity into the data set instead of in the test cases, you usually just ossify the algorithm's current results. It seems efficient, but I find you get something that's hard to maintain not just because the test is fragile, but also because the test doesn't express why a particular value is expected for a particular item. [09:45] jtv: I am on the phone at the moment, ttyl. === 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 [10:45] gmb: care to take a look at an MP I re-submitted? === jpds changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: || queue [jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [10:49] adeuring: Would that be the one for bug 172501? [10:49] Bug #172501: reject non-code patch attachements

[10:49] If so, I've just approved it :) [10:49] gmb: yes [10:49] gmb: thanks! [10:49] np === matsubara-afk is now known as matsubara [11:27] hi gmb! [11:27] 3 cheers for our ocr :) [11:28] (No, I am _not_ softening you up for an oversized review. Why would you think that?) [11:28] jtv: Ohai. And thank you... But usually that kind of cheerleading precedes ... ah [11:28] the wheels of intellect grind slowly but surely [11:29] jtv: Well, to be fair, if you'd been bigjools or any of the other soyuz guys, I would've logged off straight away [11:29] "No, I can't look at your mad 16,000-line abomination... by grandmother's on fire" or something [11:29] ...and that's why I was very explicit about not needing an oversized review. [11:29] Fair dos. [11:31] ts .. ts .. as if we Soyuz guys *ever* submitted oversized branches for review [11:31] gmb: now, I gather that the aforementioned scenario is not new to you. Has it gotten to the point where they ask "maternal or paternal"? [11:31] :) [11:32] Not quite. [11:32] But ever since cprov asked me to do a review in the back of a van at 40mph I've been leery of Soyuz code... [11:32] lol [11:33] IIRC the conversation went "I may vomit on your laptop, Celso" ... "It's okay, it's a thinkpad, it's vomit-proof." [11:33] cprov was not the top branch submitter for no reason :) [11:33] gmb: soyuz and rosetta are basically the place where dinosaurs still roam. Dealing with them is a special skill, but not one that endears one to reviewers. [11:34] gmb: but your nausea was not caused by his code .. or ..? [11:34] al-maisan: probably the beating, gagging, drugging & manic driving that was involved in getting him to review the branch. [11:34] al-maisan: I think it was actually the large steak I'd just eaten, courtesy of sg's corporate credit card... could be wrong, though [11:35] gmb: ah, November Nexus ;) [11:35] Got it in one :) [11:35] al-maisan: on an unrelated note... any reactions to what I said about the unit tests? (I warn you, government bureaucracy just put me in a foul mood :) [11:36] jtv: you may be theoretically right but it would a huge quantum of pain to restructure the tests [11:37] al-maisan: I'm not asking you to restructure all the existing tests; I am an _understanding_ fascist. But that one big test you added... [11:37] jtv: also .. the longish tests allow me to exercise certain scenarios w/o having to go through the set-up over and over again [11:38] * al-maisan looks at the added test again [11:38] al-maisan: don't fall for that temptation! I speak from bitter experience with the other great land-before-time codebase in LP [11:38] jtv: thank you for being so understanding ;) [11:38] hrmm .. hmmm [11:40] al-maisan: the trick is to find a way to make repeating the setup easy for _you_, and then repeat it as often as necessary. Don't spend too much time worrying about test time just now; this is too important. [11:40] jtv: right .. I'll get back to you when I have something. [11:41] al-maisan: thanks... you're becoming frighteningly experienced in dealing with my foul moods [11:41] hrm .. fascist reviewers .. mumble .. [11:42] jawohl! [11:42] :) [11:42] Code, you allied pigdog! [11:42] Zu Befehl! [11:42] o/ [11:42] hey, it works for that as well! [11:42] o\ [11:43] lol === 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 === noodles775 changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: lunch || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [12:05] Hi gmb [12:05] Can I get you to look at a small update to a branch you already reviewed last week: https://code.edge.launchpad.net/~michael.nelson/launchpad/510331-syncsources-latest-pub/+merge/18070 === mrevell is now known as mrevellunch [13:06] noodles775: Sure === 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 [13:06] Ta. [13:09] jtv: thanks for your explanation on the imports [13:09] noodles775: I generally prefer `if not result_set.is_empty()` to `if result_set.count() > 0`, FWIW, but I'm not too fussed either way about it - just a thought. [13:09] bac: np [13:10] noodles775: Other than that it looks fine. I'll add another review. [13:10] gmb: thanks. [13:10] gmb: you may avoid performance problems that way as well, if the data set is very large [13:10] noodles775: jtv Speaks only in truths. [13:10] * jtv bows orientally [13:10] :) === 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 [13:13] gmb: i am CHRing this week so i will defer my OCR unless things get really backed up [13:14] bac: Righto. === mrevellunch is now known as mrevel === mrevel is now known as mrevell [13:40] gmb, jtv: http://pastebin.ubuntu.com/367524/ [13:40] Looks like is_empty isn't included on the ISQLObjectResultSet in storm.zope.interfaces :/ [13:41] ?! [13:41] Oh, ah. [13:41] I see. [13:41] That's suboptimal. [13:43] noodles775: In that case, just go back to using .count(). You could maybe file a tech-debt bug about changing getPublishedSources() to use Storm, but I don't know how worthwhile that would be. [13:43] Yep, thanks. [13:44] jml: Want someone to review your subunit-by-default branch? I've basically done it because I was interested. Hope you don't mind :) [13:49] jtv: going through last night's review comments, don't quite see how [13:49] "COALESCE(buildqueue.virtualized, TRUE) = %(virtualized)s" [13:49] is equivalent to [13:49] "buildqueue.virtualized = %(virtualized)s OR (buildqueue.virtualized IS NULL AND %(virtualized)s = TRUE" [13:50] al-maisan: it's not entirely obvious, I'll admit—but can you find a spot in the truth tables where they differ? [13:51] * al-maisan did not try that yet :P [13:52] allenap, thanks. [14:01] jtv: that's a clever simplification, thanks again! [14:01] np === jamalta-afk is now known as jamalta [14:15] Hi jpds; bac pointed out that I accidentally bumped your review from the queue earlier on. My apologies. I'll take a look at it shortly. === gmb changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: jpds || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [14:15] gmb: No problem, thanks. [14:16] jpds: If I ever do that again feel free to just ping me. I suck at managing channel topics. [14:16] (You're not the first to suffer that fate, and you won't be the last, I'm sure...) [14:17] jml: I reviewed it, but only just noticed that you'd marked it as work in progress. Sorry if I jumped the gun there, but I'm happy to review incrementals. [14:19] jml: If you have time today, could you take another squizz at https://code.edge.launchpad.net/~gmb/launchpad/blob-processing-job-table-bug-513762/+merge/18218 please? If you've got a suggestion for a better table name I'm all ears. [14:22] jtv: I took care of all of your review comments but the last one, the resulting changes are here: http://pastebin.ubuntu.com/367546/ [14:23] jpds: With whom di you have a pre-implementation call or chat about this branch, btw? Your cover letter is kinda sparse... [14:23] gmb: I had a talk with sinzui about it. [14:23] al-maisan: thanks! otp but will get to it in a moment [14:23] jpds: Cool. [14:24] jpds: Normally, you should include the following in the cover letter to make the reviewer's life easier: Pre-imp discussion details, lint check (run `make lint` in the tree) and details of why you've made the changes you have. [14:25] jpds: So, I might well have some questions for you... ;) [14:25] gmb: That's fine, I'll remember that for next time. [14:25] Cool. === salgado is now known as salgado-lunch === matsubara is now known as matsubara-lunch [14:57] jpds: Looks good. r=me. Want me to run it through ec2 for you? [14:59] s/Want me to/I will/ :) [15:00] gmb: Yes, please. [15:00] jpds: It's on its way now. === 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 [15:11] jtv: .. and last but not least, here's the big unit test split up: http://pastebin.ubuntu.com/367637/ [15:12] wow! [15:13] al-maisan: about the get_one() on the result set... that's the equivalent of first(), not of one(), right? I mean, it doesn't break when you've got more than one row? [15:13] (I think this is right, but better to ask) [15:14] That disable-all-native-builders loop sure turned out easier than expected... so did a lot of the test splitup [15:14] jtv: there will never be more than one row due to the "ORDER BY lastscore DESC, job LIMIT 1" clause [15:15] ah ok :) [15:15] al-maisan: assuming tests pass, r=me. I'm off to do the paperwork. [15:15] Thanks for bearing with me. [15:16] jtv: they do pass indeed. Thanks for the review! [15:18] np [15:23] jtv: could you please "bless" this one last change (http://pastebin.ubuntu.com/367645/): "Now processors are only retrieved once and reused throughout the test code." [15:24] al-maisan: looks like an improvement, though it does beg the question why these aren't loops. :-) [15:25] That said, this is clearly an improvement and I'll bless it. [15:26] jtv: thank you! [15:27] np === adiroiban changed the topic of #launchpad-reviews to: on-call: gmb || reviewing: - || queue [adiroiban(bug-340664)] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [15:34] al-maisan: hi. Do you have time to land this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-509252/+merge/18184 ? Thanks! [15:49] adiroiban: later this evening .. is that agreeable? === salgado-lunch is now known as salgado [16:03] al-maisan: no hurry. I just wante to know if it's ok for you to land it or if should I ask someone else. [16:03] thanks! === matsubara-lunch is now known as matsubara === jamalta is now known as jamalta-afk [16:03] adiroiban: no problem === jamalta-afk is now known as jamalta === gmb 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 === henninge_ is now known as henninge [16:20] Hi! ;) [16:21] Is there a reviewer around to approve my cool improvement to jml's cool on-edge script? === henninge changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [16:21] https://code.edge.launchpad.net/~henninge/launchpad/on-edge/+merge/18454 [17:04] jtv: would you mind reviewing that great addition to the on-edge script? [17:04] henninge: where? [17:04] https://code.edge.launchpad.net/~henninge/launchpad/on-edge/+merge/18454 [17:42] Would anybody like a simple text change review? https://code.edge.launchpad.net/~matthew.revell/launchpad/bug-subscribe-help-bug-484297/+merge/18458 === jpds changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [henninge,jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [17:54] mrevell: I know allenap already approved this but shouldn't full sentences end in a full stop? [17:54] jtv: did you see my link to the MP? [17:54] henninge, I'm under the impression that our style guide forbids full stops in tool-tips. I'll check that [17:54] henninge: yes, I'm just reading the original announcement for on-edge. [17:55] jtv: cool, thanks [17:55] mrevell: as long as we are consistent about it ... ;-) [18:11] henninge: "res" is not a very clear variable name... in Catalan it means "nothing" IIRC whereas in Latin it means "thing." How about "match"? [18:12] jtv: result would be better, then. [18:12] jtv: no, match is ok [18:12] * henninge thought the class was called "Result" but that is in storm ... [18:12] ;-) [18:12] henninge: btw that script doesn't work for me because it doesn't honour LP_TRUNK_NAME. :( [18:13] jtv: I am sure it can take a lot of improvements ... [18:14] henninge: done [18:17] rockstar, https://code.edge.launchpad.net/~abentley/launchpad/empty-conflicts/+merge/18460 [18:22] adeuring: okay, got that conditional working; about to commit. Ratio of coding time to testing (startup/shutdown/etc) time: appx 1 to 4 :-(. [18:22] kfogel: that's not unusual ;) [18:23] adeuring: and I haven't even gotten a story test written for it yet. I'm going to leave an XXX for that, due to Jorge's deadline. [18:23] kfogel: yeah, considering our time constraints, that should be ok [18:29] jtv: thanks a lot. [18:31] np === matsubara is now known as matsubara-afk === salgado is now known as salgado-afk [22:32] mwhudson, could you do a review for me? [22:33] rockstar: sure [22:33] mwhudson, https://code.edge.launchpad.net/~rockstar/launchpad/default-branch-upgrade-bad/+merge/18483 [22:34] Wow, Launchpad is being doggedly slow today. [22:41] https://code.edge.launchpad.net/~james-w/launchpad/i-love-sync-source/+merge/18485 pls [22:43] rockstar: done [22:44] * mwhudson looks at james_w's [22:44] thanks mwhudson [22:45] james_w: basically, can i trust you enough to be sure this is a good change? [22:45] heh, I'd hope so [22:45] james_w: also, i presume this isn't tested... [22:45] I refer you to the quotes page for that [22:45] right [22:46] "-a" is a batch run which tries to sync eligible things, so it doesn't want to print 16000 lines [22:46] james_w: do you want me to land it too? [22:46] so it skips it unless you pass --moreverbose [22:46] james_w: can you set a commit message? [22:46] but I'm saying "sync this package please", and it's refusing to as I made a mistake, but not telling me why [22:46] fair enough [22:46] yes please and sure [22:48] done [22:52] * mwhudson hugs "ec2 land" === jamalta is now known as jamalta-afk