/srv/irclogs.ubuntu.com/2010/02/02/#launchpad-reviews.txt

jtvVery strict reviewer wanted for 1-line change!  https://code.launchpad.net/~jtv/launchpad/bug-515702/+merge/1841109:08
* al-maisan is not very strict but looks nevertheless :)09:22
al-maisanjtv: r=me09:24
jtval-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 looks09:25
al-maisanjtv: you're right, I'll revise the code to return the (currently unused) default if there is no head job09:27
jtvthanks09:28
jpdsI have https://code.edge.launchpad.net/~jpds/launchpad/fix_515388/+merge/18413 if anyone has time. :)09:28
jtv...tense silence...09:30
jtv:-)09:30
jtval-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
jtval-maisan: or is that data set up by setUp?09:30
al-maisanjtv: the latter09:31
jtvoic09:31
al-maisanthe tests in test_buildqueue.py do not use any sample data09:31
jtvright ho09:32
jtvstill, 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
jtvBecause if it works for 3 queue items, chances are it's going to work for these 18 as well.09:35
jtvWhen 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:37
al-maisanjtv: I am on the phone at the moment, ttyl.09:45
=== 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
adeuringgmb: care to take a look at an MP I re-submitted?10:45
=== 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
gmbadeuring: Would that be the one for bug 172501?10:49
mupBug #172501: reject non-code patch attachements <patch-tracking> <story-patch-report> <Launchpad Bugs:In Progress by adeuring> <https://launchpad.net/bugs/172501>10:49
gmbIf so, I've just approved it :)10:49
adeuringgmb: yes10:49
adeuringgmb: thanks!10:49
gmbnp10:49
=== matsubara-afk is now known as matsubara
jtvhi gmb!11:27
jtv3 cheers for our ocr  :)11:27
jtv(No, I am _not_ softening you up for an oversized review.  Why would you think that?)11:28
gmbjtv: Ohai. And thank you... But usually that kind of cheerleading precedes ... ah11:28
jtvthe wheels of intellect grind slowly but surely11:28
gmbjtv: Well, to be fair, if you'd been bigjools or any of the other soyuz guys, I would've logged off straight away11:29
gmb"No, I can't look at your mad 16,000-line abomination... by grandmother's on fire" or something11:29
jtv...and that's why I was very explicit about not needing an oversized review.11:29
gmbFair dos.11:29
al-maisants .. ts .. as if we Soyuz guys *ever* submitted oversized branches for review11:31
jtvgmb: 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:31
gmbNot quite.11:32
gmbBut 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
noodles775lol11:32
gmbIIRC the conversation went "I may vomit on your laptop, Celso" ... "It's okay, it's  a thinkpad, it's vomit-proof."11:33
al-maisancprov was not the top branch submitter for no reason :)11:33
jtvgmb: 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:33
al-maisangmb: but your nausea was not caused by his code .. or ..?11:34
jtval-maisan: probably the beating, gagging, drugging & manic driving that was involved in getting him to review the branch.11:34
gmbal-maisan: I think it was actually the large steak I'd just eaten, courtesy of sg's corporate credit card... could be wrong, though11:34
al-maisangmb: ah, November Nexus ;)11:35
gmbGot it in one :)11:35
jtval-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:35
al-maisanjtv: you may be theoretically right but it would a huge quantum of pain to restructure the tests11:36
jtval-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-maisanjtv: also .. the longish tests allow me to exercise certain scenarios w/o having to go through the set-up over and over again11:37
* al-maisan looks at the added test again11:38
jtval-maisan: don't fall for that temptation!  I speak from bitter experience with the other great land-before-time codebase in LP11:38
al-maisanjtv: thank you for being so understanding ;)11:38
al-maisanhrmm .. hmmm11:38
jtval-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-maisanjtv: right .. I'll get back to you when I have something.11:40
jtval-maisan: thanks... you're becoming frighteningly experienced in dealing with my foul moods11:41
al-maisanhrm .. fascist reviewers .. mumble ..11:41
jtvjawohl!11:42
al-maisan:)11:42
jtvCode, you allied pigdog!11:42
al-maisanZu Befehl!11:42
jtvo/11:42
jtvhey, it works for that as well!11:42
al-maisano\11:42
jtvlol11:43
=== 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
noodles775Hi gmb12:05
noodles775Can 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/1807012:05
=== mrevell is now known as mrevellunch
gmbnoodles775: Sure13:06
=== 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
noodles775Ta.13:06
bacjtv: thanks for your explanation on the imports13:09
gmbnoodles775: 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
jtvbac: np13:09
gmbnoodles775: Other than that it looks fine. I'll add another review.13:10
noodles775gmb: thanks.13:10
jtvgmb: you may avoid performance problems that way as well, if the data set is very large13:10
gmbnoodles775: jtv Speaks only in truths.13:10
* jtv bows orientally13:10
noodles775:)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
bacgmb: i am CHRing this week so i will defer my OCR unless things get really backed up13:13
gmbbac: Righto.13:14
=== mrevellunch is now known as mrevel
=== mrevel is now known as mrevell
noodles775gmb, jtv: http://pastebin.ubuntu.com/367524/13:40
noodles775Looks like is_empty isn't included on the ISQLObjectResultSet in storm.zope.interfaces :/13:40
gmb?!13:41
gmbOh, ah.13:41
gmbI see.13:41
gmbThat's suboptimal.13:41
gmbnoodles775: 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
noodles775Yep, thanks.13:43
allenapjml: Want someone to review your subunit-by-default branch? I've basically done it because I was interested. Hope you don't mind :)13:44
al-maisanjtv: going through last night's review comments, don't quite see how13:49
al-maisan"COALESCE(buildqueue.virtualized, TRUE) = %(virtualized)s"13:49
al-maisanis equivalent to13:49
al-maisan"buildqueue.virtualized = %(virtualized)s OR (buildqueue.virtualized IS NULL AND %(virtualized)s = TRUE"13:49
jtval-maisan: it's not entirely obvious, I'll admit—but can you find a spot in the truth tables where they differ?13:50
* al-maisan did not try that yet :P13:51
jmlallenap, thanks.13:52
al-maisanjtv: that's a clever simplification, thanks again!14:01
jtvnp14:01
=== jamalta-afk is now known as jamalta
gmbHi 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
=== 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
jpdsgmb: No problem, thanks.14:15
gmbjpds: 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:16
allenapjml: 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:17
gmbjml: 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:19
al-maisanjtv: I took care of all of your review comments but the last one, the resulting changes are here: http://pastebin.ubuntu.com/367546/14:22
gmbjpds: With whom di you have a pre-implementation call or chat about this branch, btw? Your cover letter is kinda sparse...14:23
jpdsgmb: I had a talk with sinzui about it.14:23
jtval-maisan: thanks!  otp but will get to it in a moment14:23
gmbjpds: Cool.14:23
gmbjpds: 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:24
gmbjpds: So, I might well have some questions for you... ;)14:25
jpdsgmb: That's fine, I'll remember that for next time.14:25
gmbCool.14:25
=== salgado is now known as salgado-lunch
=== matsubara is now known as matsubara-lunch
gmbjpds: Looks good. r=me. Want me to run it through ec2 for you?14:57
gmbs/Want me to/I will/ :)14:59
jpdsgmb: Yes, please.15:00
gmbjpds: It's on its way now.15:00
=== 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
al-maisanjtv:  .. and last but not least, here's the big unit test split up: http://pastebin.ubuntu.com/367637/15:11
jtvwow!15:12
jtval-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:13
jtvThat disable-all-native-builders loop sure turned out easier than expected... so did a lot of the test splitup15:14
al-maisanjtv: there will never be more than one row due to the "ORDER BY lastscore DESC, job LIMIT 1" clause15:14
jtvah ok :)15:15
jtval-maisan: assuming tests pass, r=me.  I'm off to do the paperwork.15:15
jtvThanks for bearing with me.15:15
al-maisanjtv: they do pass indeed. Thanks for the review!15:16
jtvnp15:18
al-maisanjtv: 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:23
jtval-maisan: looks like an improvement, though it does beg the question why these aren't loops.  :-)15:24
jtvThat said, this is clearly an improvement and I'll bless it.15:25
al-maisanjtv: thank you!15:26
jtvnp15:27
=== 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
adiroibanal-maisan: hi. Do you have time to land this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-509252/+merge/18184 ? Thanks!15:34
al-maisanadiroiban: later this evening .. is that agreeable?15:49
=== salgado-lunch is now known as salgado
adiroibanal-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
adiroibanthanks!16:03
=== matsubara-lunch is now known as matsubara
=== jamalta is now known as jamalta-afk
al-maisanadiroiban: no problem16:03
=== 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
henningeHi! ;)16:20
henningeIs there a reviewer around to approve my cool improvement to jml's cool on-edge script?16:21
=== 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
henningehttps://code.edge.launchpad.net/~henninge/launchpad/on-edge/+merge/1845416:21
henningejtv: would you mind reviewing that great addition to the on-edge script?17:04
jtvhenninge: where?17:04
henningehttps://code.edge.launchpad.net/~henninge/launchpad/on-edge/+merge/1845417:04
mrevellWould anybody like a simple text change review? https://code.edge.launchpad.net/~matthew.revell/launchpad/bug-subscribe-help-bug-484297/+merge/1845817:42
=== 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
henningemrevell: I know allenap already approved this but shouldn't full sentences end in a full stop?17:54
henningejtv: did you see my link to the MP?17:54
mrevellhenninge, I'm under the impression that our style guide forbids full stops in tool-tips. I'll check that17:54
jtvhenninge: yes, I'm just reading the original announcement for on-edge.17:54
henningejtv: cool, thanks17:55
henningemrevell: as long as we are consistent about it ... ;-)17:55
jtvhenninge: "res" is not a very clear variable name... in Catalan it means "nothing" IIRC whereas in Latin it means "thing."  How about "match"?18:11
henningejtv: result would be better, then.18:12
henningejtv: no, match is ok18:12
* henninge thought the class was called "Result" but that is in storm ...18:12
henninge;-)18:12
jtvhenninge: btw that script doesn't work for me because it doesn't honour LP_TRUNK_NAME.  :(18:12
henningejtv: I am sure it can take a lot of improvements ...18:13
jtvhenninge: done18:14
abentleyrockstar, https://code.edge.launchpad.net/~abentley/launchpad/empty-conflicts/+merge/1846018:17
kfogeladeuring: okay, got that conditional working; about to commit.  Ratio of coding time to testing (startup/shutdown/etc) time: appx 1 to 4 :-(.18:22
adeuringkfogel: that's not unusual ;)18:22
kfogeladeuring: 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
adeuringkfogel: yeah, considering our time constraints, that should be ok18:23
henningejtv: thanks a lot.18:29
jtvnp18:31
=== matsubara is now known as matsubara-afk
=== salgado is now known as salgado-afk
rockstarmwhudson, could you do a review for me?22:32
mwhudsonrockstar: sure22:33
rockstarmwhudson, https://code.edge.launchpad.net/~rockstar/launchpad/default-branch-upgrade-bad/+merge/1848322:33
rockstarWow, Launchpad is being doggedly slow today.22:34
james_whttps://code.edge.launchpad.net/~james-w/launchpad/i-love-sync-source/+merge/18485 pls22:41
mwhudsonrockstar: done22:43
* mwhudson looks at james_w's22:44
james_wthanks mwhudson22:44
mwhudsonjames_w: basically, can i trust you enough to be sure this is a good change?22:45
james_wheh, I'd hope so22:45
mwhudsonjames_w: also, i presume this isn't tested...22:45
james_wI refer you to the quotes page for that22:45
mwhudsonright22:45
james_w"-a" is a batch run which tries to sync eligible things, so it doesn't want to print 16000 lines22:46
mwhudsonjames_w: do you want me to land it too?22:46
james_wso it skips it unless you pass --moreverbose22:46
mwhudsonjames_w: can you set a commit message?22:46
james_wbut I'm saying "sync this package please", and it's refusing to as I made a mistake, but not telling me why22:46
mwhudsonfair enough22:46
james_wyes please and sure22:46
james_wdone22:48
* mwhudson hugs "ec2 land"22:52
=== jamalta is now known as jamalta-afk

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!