[09:08] <jtv> 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] <al-maisan> jtv: r=me
[09:25] <jtv> 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] <al-maisan> jtv: you're right, I'll revise the code to return the (currently unused) default if there is no head job
[09:28] <jtv> thanks
[09:28] <jpds> I have https://code.edge.launchpad.net/~jpds/launchpad/fix_515388/+merge/18413 if anyone has time. :)
[09:30] <jtv> ...tense silence...
[09:30] <jtv> :-)
[09:30] <jtv> 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] <jtv> al-maisan: or is that data set up by setUp?
[09:31] <al-maisan> jtv: the latter
[09:31] <jtv> oic
[09:31] <al-maisan> the tests in test_buildqueue.py do not use any sample data
[09:32] <jtv> right ho
[09:35] <jtv> 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] <jtv> Because if it works for 3 queue items, chances are it's going to work for these 18 as well.
[09:37] <jtv> 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] <al-maisan> jtv: I am on the phone at the moment, ttyl.
[10:45] <adeuring> gmb: care to take a look at an MP I re-submitted?
[10:49] <gmb> adeuring: Would that be the one for bug 172501?
[10:49] <mup> Bug #172501: reject non-code patch attachements <patch-tracking> <story-patch-report> <Launchpad Bugs:In Progress by adeuring> <https://launchpad.net/bugs/172501>
[10:49] <gmb> If so, I've just approved it :)
[10:49] <adeuring> gmb: yes
[10:49] <adeuring> gmb: thanks!
[10:49] <gmb> np
[11:27] <jtv> hi gmb!
[11:27] <jtv> 3 cheers for our ocr  :)
[11:28] <jtv> (No, I am _not_ softening you up for an oversized review.  Why would you think that?)
[11:28] <gmb> jtv: Ohai. And thank you... But usually that kind of cheerleading precedes ... ah
[11:28] <jtv> the wheels of intellect grind slowly but surely
[11:29] <gmb> 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] <gmb> "No, I can't look at your mad 16,000-line abomination... by grandmother's on fire" or something
[11:29] <jtv> ...and that's why I was very explicit about not needing an oversized review.
[11:29] <gmb> Fair dos.
[11:31] <al-maisan> ts .. ts .. as if we Soyuz guys *ever* submitted oversized branches for review
[11:31] <jtv> 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] <al-maisan> :)
[11:32] <gmb> Not quite.
[11:32] <gmb> 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] <noodles775> lol
[11:33] <gmb> IIRC the conversation went "I may vomit on your laptop, Celso" ... "It's okay, it's  a thinkpad, it's vomit-proof."
[11:33] <al-maisan> cprov was not the top branch submitter for no reason :)
[11:33] <jtv> 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] <al-maisan> gmb: but your nausea was not caused by his code .. or ..?
[11:34] <jtv> al-maisan: probably the beating, gagging, drugging & manic driving that was involved in getting him to review the branch.
[11:34] <gmb> 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] <al-maisan> gmb: ah, November Nexus ;)
[11:35] <gmb> Got it in one :)
[11:35] <jtv> 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] <al-maisan> jtv: you may be theoretically right but it would a huge quantum of pain to restructure the tests
[11:37] <jtv> 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] <al-maisan> 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] <jtv> 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] <al-maisan> jtv: thank you for being so understanding ;)
[11:38] <al-maisan> hrmm .. hmmm
[11:40] <jtv> 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] <al-maisan> jtv: right .. I'll get back to you when I have something.
[11:41] <jtv> al-maisan: thanks... you're becoming frighteningly experienced in dealing with my foul moods
[11:41] <al-maisan> hrm .. fascist reviewers .. mumble ..
[11:42] <jtv> jawohl!
[11:42] <al-maisan> :)
[11:42] <jtv> Code, you allied pigdog!
[11:42] <al-maisan> Zu Befehl!
[11:42] <jtv> o/
[11:42] <jtv> hey, it works for that as well!
[11:42] <al-maisan> o\
[11:43] <jtv> lol
[12:05] <noodles775> Hi gmb
[12:05] <noodles775> 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
[13:06] <gmb> noodles775: Sure
[13:06] <noodles775> Ta.
[13:09] <bac> jtv: thanks for your explanation on the imports
[13:09] <gmb> 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] <jtv> bac: np
[13:10] <gmb> noodles775: Other than that it looks fine. I'll add another review.
[13:10] <noodles775> gmb: thanks.
[13:10] <jtv> gmb: you may avoid performance problems that way as well, if the data set is very large
[13:10] <gmb> noodles775: jtv Speaks only in truths.
[13:10]  * jtv bows orientally
[13:10] <noodles775> :)
[13:13] <bac> gmb: i am CHRing this week so i will defer my OCR unless things get really backed up
[13:14] <gmb> bac: Righto.
[13:40] <noodles775> gmb, jtv: http://pastebin.ubuntu.com/367524/
[13:40] <noodles775> Looks like is_empty isn't included on the ISQLObjectResultSet in storm.zope.interfaces :/
[13:41] <gmb> ?!
[13:41] <gmb> Oh, ah.
[13:41] <gmb> I see.
[13:41] <gmb> That's suboptimal.
[13:43] <gmb> 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] <noodles775> Yep, thanks.
[13:44] <allenap> 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] <al-maisan> jtv: going through last night's review comments, don't quite see how
[13:49] <al-maisan> "COALESCE(buildqueue.virtualized, TRUE) = %(virtualized)s"
[13:49] <al-maisan> is equivalent to
[13:49] <al-maisan> "buildqueue.virtualized = %(virtualized)s OR (buildqueue.virtualized IS NULL AND %(virtualized)s = TRUE"
[13:50] <jtv> 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] <jml> allenap, thanks.
[14:01] <al-maisan> jtv: that's a clever simplification, thanks again!
[14:01] <jtv> np
[14:15] <gmb> 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.
[14:15] <jpds> gmb: No problem, thanks.
[14:16] <gmb> jpds: If I ever do that again feel free to just ping me. I suck at managing channel topics.
[14:16] <gmb> (You're not the first to suffer that fate, and you won't be the last, I'm sure...)
[14:17] <allenap> 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] <gmb> 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] <al-maisan> 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] <gmb> jpds: With whom di you have a pre-implementation call or chat about this branch, btw? Your cover letter is kinda sparse...
[14:23] <jpds> gmb: I had a talk with sinzui about it.
[14:23] <jtv> al-maisan: thanks!  otp but will get to it in a moment
[14:23] <gmb> jpds: Cool.
[14:24] <gmb> 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] <gmb> jpds: So, I might well have some questions for you... ;)
[14:25] <jpds> gmb: That's fine, I'll remember that for next time.
[14:25] <gmb> Cool.
[14:57] <gmb> jpds: Looks good. r=me. Want me to run it through ec2 for you?
[14:59] <gmb> s/Want me to/I will/ :)
[15:00] <jpds> gmb: Yes, please.
[15:00] <gmb> jpds: It's on its way now.
[15:11] <al-maisan> jtv:  .. and last but not least, here's the big unit test split up: http://pastebin.ubuntu.com/367637/
[15:12] <jtv> wow!
[15:13] <jtv> 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] <jtv> (I think this is right, but better to ask)
[15:14] <jtv> That disable-all-native-builders loop sure turned out easier than expected... so did a lot of the test splitup
[15:14] <al-maisan> jtv: there will never be more than one row due to the "ORDER BY lastscore DESC, job LIMIT 1" clause
[15:15] <jtv> ah ok :)
[15:15] <jtv> al-maisan: assuming tests pass, r=me.  I'm off to do the paperwork.
[15:15] <jtv> Thanks for bearing with me.
[15:16] <al-maisan> jtv: they do pass indeed. Thanks for the review!
[15:18] <jtv> np
[15:23] <al-maisan> 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] <jtv> al-maisan: looks like an improvement, though it does beg the question why these aren't loops.  :-)
[15:25] <jtv> That said, this is clearly an improvement and I'll bless it.
[15:26] <al-maisan> jtv: thank you!
[15:27] <jtv> np
[15:34] <adiroiban> 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] <al-maisan> adiroiban: later this evening .. is that agreeable?
[16:03] <adiroiban> 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] <adiroiban> thanks!
[16:03] <al-maisan> adiroiban: no problem
[16:20] <henninge> Hi! ;)
[16:21] <henninge> Is there a reviewer around to approve my cool improvement to jml's cool on-edge script?
[16:21] <henninge> https://code.edge.launchpad.net/~henninge/launchpad/on-edge/+merge/18454
[17:04] <henninge> jtv: would you mind reviewing that great addition to the on-edge script?
[17:04] <jtv> henninge: where?
[17:04] <henninge> https://code.edge.launchpad.net/~henninge/launchpad/on-edge/+merge/18454
[17:42] <mrevell> Would anybody like a simple text change review? https://code.edge.launchpad.net/~matthew.revell/launchpad/bug-subscribe-help-bug-484297/+merge/18458
[17:54] <henninge> mrevell: I know allenap already approved this but shouldn't full sentences end in a full stop?
[17:54] <henninge> jtv: did you see my link to the MP?
[17:54] <mrevell> henninge, I'm under the impression that our style guide forbids full stops in tool-tips. I'll check that
[17:54] <jtv> henninge: yes, I'm just reading the original announcement for on-edge.
[17:55] <henninge> jtv: cool, thanks
[17:55] <henninge> mrevell: as long as we are consistent about it ... ;-)
[18:11] <jtv> 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] <henninge> jtv: result would be better, then.
[18:12] <henninge> jtv: no, match is ok
[18:12]  * henninge thought the class was called "Result" but that is in storm ...
[18:12] <henninge> ;-)
[18:12] <jtv> henninge: btw that script doesn't work for me because it doesn't honour LP_TRUNK_NAME.  :(
[18:13] <henninge> jtv: I am sure it can take a lot of improvements ...
[18:14] <jtv> henninge: done
[18:17] <abentley> rockstar, https://code.edge.launchpad.net/~abentley/launchpad/empty-conflicts/+merge/18460
[18:22] <kfogel> 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] <adeuring> kfogel: that's not unusual ;)
[18:23] <kfogel> 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] <adeuring> kfogel: yeah, considering our time constraints, that should be ok
[18:29] <henninge> jtv: thanks a lot.
[18:31] <jtv> np
[22:32] <rockstar> mwhudson, could you do a review for me?
[22:33] <mwhudson> rockstar: sure
[22:33] <rockstar> mwhudson, https://code.edge.launchpad.net/~rockstar/launchpad/default-branch-upgrade-bad/+merge/18483
[22:34] <rockstar> Wow, Launchpad is being doggedly slow today.
[22:41] <james_w> https://code.edge.launchpad.net/~james-w/launchpad/i-love-sync-source/+merge/18485 pls
[22:43] <mwhudson> rockstar: done
[22:44]  * mwhudson looks at james_w's
[22:44] <james_w> thanks mwhudson
[22:45] <mwhudson> james_w: basically, can i trust you enough to be sure this is a good change?
[22:45] <james_w> heh, I'd hope so
[22:45] <mwhudson> james_w: also, i presume this isn't tested...
[22:45] <james_w> I refer you to the quotes page for that
[22:45] <mwhudson> right
[22:46] <james_w> "-a" is a batch run which tries to sync eligible things, so it doesn't want to print 16000 lines
[22:46] <mwhudson> james_w: do you want me to land it too?
[22:46] <james_w> so it skips it unless you pass --moreverbose
[22:46] <mwhudson> james_w: can you set a commit message?
[22:46] <james_w> 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] <mwhudson> fair enough
[22:46] <james_w> yes please and sure
[22:48] <james_w> done
[22:52]  * mwhudson hugs "ec2 land"