jtv | Very strict reviewer wanted for 1-line change! https://code.launchpad.net/~jtv/launchpad/bug-515702/+merge/18411 | 09:08 |
---|---|---|
* al-maisan is not very strict but looks nevertheless :) | 09:22 | |
al-maisan | jtv: r=me | 09:24 |
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:25 | |
al-maisan | jtv: you're right, I'll revise the code to return the (currently unused) default if there is no head job | 09:27 |
jtv | thanks | 09:28 |
jpds | I 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 |
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:30 |
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:31 |
jtv | right ho | 09:32 |
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:35 |
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:37 |
al-maisan | jtv: 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 | ||
adeuring | gmb: 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 | ||
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 | 10:49 |
=== matsubara-afk is now known as matsubara | ||
jtv | hi gmb! | 11:27 |
jtv | 3 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 |
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:28 |
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:29 |
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:31 |
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:32 |
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:33 |
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:34 |
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:35 |
al-maisan | jtv: you may be theoretically right but it would a huge quantum of pain to restructure the tests | 11:36 |
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:37 |
* 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:38 |
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:40 |
jtv | al-maisan: thanks... you're becoming frighteningly experienced in dealing with my foul moods | 11:41 |
al-maisan | hrm .. fascist reviewers .. mumble .. | 11:41 |
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:42 |
jtv | lol | 11: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 | ||
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 | 12:05 |
=== mrevell is now known as mrevellunch | ||
gmb | noodles775: Sure | 13: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 | ||
noodles775 | Ta. | 13:06 |
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:09 |
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: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 | ||
bac | gmb: i am CHRing this week so i will defer my OCR unless things get really backed up | 13:13 |
gmb | bac: Righto. | 13:14 |
=== mrevellunch is now known as mrevel | ||
=== mrevel is now known as mrevell | ||
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:40 |
gmb | ?! | 13:41 |
gmb | Oh, ah. | 13:41 |
gmb | I see. | 13:41 |
gmb | That's suboptimal. | 13:41 |
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:43 |
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:44 |
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:49 |
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:50 |
* al-maisan did not try that yet :P | 13:51 | |
jml | allenap, thanks. | 13:52 |
al-maisan | jtv: that's a clever simplification, thanks again! | 14:01 |
jtv | np | 14:01 |
=== jamalta-afk is now known as jamalta | ||
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 |
=== 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 | ||
jpds | gmb: No problem, thanks. | 14:15 |
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:16 |
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:17 |
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:19 |
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:22 |
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:23 |
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:24 |
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:25 |
=== salgado is now known as salgado-lunch | ||
=== matsubara is now known as matsubara-lunch | ||
gmb | jpds: Looks good. r=me. Want me to run it through ec2 for you? | 14:57 |
gmb | s/Want me to/I will/ :) | 14:59 |
jpds | gmb: Yes, please. | 15:00 |
gmb | jpds: 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-maisan | jtv: .. and last but not least, here's the big unit test split up: http://pastebin.ubuntu.com/367637/ | 15:11 |
jtv | wow! | 15:12 |
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:13 |
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:14 |
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:15 |
al-maisan | jtv: they do pass indeed. Thanks for the review! | 15:16 |
jtv | np | 15:18 |
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:23 |
jtv | al-maisan: looks like an improvement, though it does beg the question why these aren't loops. :-) | 15:24 |
jtv | That said, this is clearly an improvement and I'll bless it. | 15:25 |
al-maisan | jtv: thank you! | 15:26 |
jtv | np | 15: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 | ||
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:34 |
al-maisan | adiroiban: later this evening .. is that agreeable? | 15:49 |
=== salgado-lunch is now known as salgado | ||
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 |
=== matsubara-lunch is now known as matsubara | ||
=== jamalta is now known as jamalta-afk | ||
al-maisan | adiroiban: no problem | 16: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 | ||
henninge | Hi! ;) | 16:20 |
henninge | Is 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 | ||
henninge | https://code.edge.launchpad.net/~henninge/launchpad/on-edge/+merge/18454 | 16:21 |
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:04 |
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: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 | ||
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:54 |
henninge | jtv: cool, thanks | 17:55 |
henninge | mrevell: as long as we are consistent about it ... ;-) | 17:55 |
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:11 |
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:12 |
henninge | jtv: I am sure it can take a lot of improvements ... | 18:13 |
jtv | henninge: done | 18:14 |
abentley | rockstar, https://code.edge.launchpad.net/~abentley/launchpad/empty-conflicts/+merge/18460 | 18:17 |
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:22 |
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:23 |
henninge | jtv: thanks a lot. | 18:29 |
jtv | np | 18:31 |
=== matsubara is now known as matsubara-afk | ||
=== salgado is now known as salgado-afk | ||
rockstar | mwhudson, could you do a review for me? | 22:32 |
mwhudson | rockstar: sure | 22:33 |
rockstar | mwhudson, https://code.edge.launchpad.net/~rockstar/launchpad/default-branch-upgrade-bad/+merge/18483 | 22:33 |
rockstar | Wow, Launchpad is being doggedly slow today. | 22:34 |
james_w | https://code.edge.launchpad.net/~james-w/launchpad/i-love-sync-source/+merge/18485 pls | 22:41 |
mwhudson | rockstar: done | 22:43 |
* mwhudson looks at james_w's | 22:44 | |
james_w | thanks mwhudson | 22:44 |
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:45 |
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:46 |
james_w | done | 22: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!