=== jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jtvthumper: you're probably eod, but if not… care to review?  https://code.edge.launchpad.net/~jtv/launchpad/bug-643345/+merge/3625807:20
jtvAnyone else who could review for me?  stub?07:30
jtvjml maybe?07:34
jtvgmb, you perhaps?07:50
jtvallenap then?07:53
StevenKjtv: I can look, but I can only give you code*07:55
jtvStevenK: theoretically that would slow down the final review, but it's not a very long branch so might as well.07:57
jtvdid noodles775 just join us?07:57
noodles775Join you for what? ;) Morning!07:57
jtvI'm shopping around for a reviewer for a CP branch… StevenK could do it, but will need a mentor to approve his review.07:58
jtvIt's this branch: https://code.edge.launchpad.net/~jtv/launchpad/bug-643345/+merge/3625807:58
noodles775jtv: maybe you could help me too? I've got an ec2 test failure email, but the tests pass locally, and the version of db-devel that was most recently merged also went through buildbot fine (r9808: https://lpbuildbot.canonical.com/builders/lucid_db_lp/builds/237)08:02
jtvnoodles775: happy to return the favour08:02
noodles775I've stared at it a bit yesterday, but would love a second pair of eyes?08:02
* jtv looks08:02
noodles775Thanks, I'll forward the failure email.08:02
jtvnoodles775: I know the feeling—I could do with a new pair of eyes myself.08:03
noodles775The email has the branch that you should be able to make and test locally. Thanks jtv08:04
StevenKnoodles775: jtv's MP doesn't have any obvious gotchas that I can see.08:06
noodles775Thanks StevenK08:06
jtvnoodles775: wow, that's not a very informative failure is it?08:07
noodles775jtv: no, and it's not just one... there's also 2 windmill errors (which also pass locally, at least did for me).08:08
jtvYes, I'm looking at them08:08
jtvnoodles775: the test failure (as opposed to the 2 errors) looks familiar08:08
noodles775I thought so too, but couldn't see any evidence of it on buildbot.08:09
jtvwe had something like that recently…  I think StevenK investigated but the problem was elusive08:09
jtvIIRC the test had just been converted from using getLatestOops (or something along those lines) to self.oopses (or something along those lines) when that happened.08:10
noodles775StevenK: http://pastebin.ubuntu.com/498260/08:10
noodles775jtv: did it also fail only on ec2 test and not on db-devel? (that would explain why I can't see it on the recent buildbot runs).08:12
noodles775jtv: And if that's the case, can you confirm for me that all 3 tests pass for you locally on that branch, and I'll submit it?08:12
StevenKFunny you should mention that: https://hudson.wedontsleep.org/job/db-devel/44/testReport/lp.services.job.tests.test_runner/TestJobRunner/test_runAll_mails_oopses/08:12
jtvnoodles775: the former is something StevenK can answer better, and the latter is something I'm about to try08:12
noodles775StevenK: Ah! So it's not just my branch! So why is it failing ec2 test and hudson, but not db-devel?08:13
StevenKnoodles775: To be completly honest, I'm not sure08:14
StevenKBut I am happy that hudson has a test failure that is useful and reproducable08:15
noodles775Different versions of oops-tools?08:15
jtvcould that make a difference to the running of the tests?08:21
jtvI thought that was all post-facto processing, not related to actual generation of oopses.08:21
jtvnoodles775: the "failure" one passes for me… trying the "error" ones.08:25
jtvStevenK: found any nits to pick yet? :)08:26
noodles775jtv: only things I've thought so far are:08:26
noodles7751) It wasn't obvious to me without reading the code below that committer_string isn't an actual commit string provided (somehow) by the committer. Not sure if there's a better name, or maybe its just me.08:27
noodles7752) In getBzrCommitterID you're using self.committer.preferredemail, even though abentley's change of the related bug title seems to indicate this is wrong?08:28
noodles775(but I don't know the background)08:28
jtvnoodles775: the first of those errored windmill tests passed for me, but the second crashed firefox.  (I'm on maverick beta)08:28
noodles775So am I... that's interesting though, I'd not run them together, always separately.08:29
jtvnoodles775: I'd be happy to come up with a better name for 1.  As for 2, that falls under the "laying groundwork" part—there must be better fixes, but for now I just want the failures out of the way.08:29
jtvnoodles775: I'm running the tests separately as well.08:29
noodles775Yes, but (regarding 2) couldn't you just remove that elif for the moment to get the failures out of the way?08:30
thumperjtv: reviewed08:30
noodles775jtv: Ah, another thought... I wonder if the windmill failures are related to the FF version.08:30
jtvnoodles775: well AIUI it's wrong _because_ it's not guaranteed to be present.08:30
jtvNot inherently wrong otherwise that I know of—people on that end made a deliberate decision to use email addresses, and I don't want to controvert them wholesale.08:31
jtvthumper: thanks!  I also have noodles775 looking at it just now08:31
thumperjtv: ok08:31
noodles775Ah, I'll just add my review so far as a comment FTR - I didn't realise thumper was/had looked at it.08:31
thumpernoodles775: that's fine08:32
jtvthumper: but very happy to have your view on it08:32
thumperI didn't read all the comments after jtv pinged08:32
jtvthere were too many :)08:32
jtvnoodles775: the branch puller test leaves a branch locked, but otherwise its test passes for me.08:32
noodles775jtv: so one windmill test errored for you... which one was it? test_pofile_new_translation_autoselect ?08:34
* noodles775 runs it locally08:35
jtvand once Firefox gets involved, I'm not even willing to guess at the moving parts and variables that come into play…08:35
jtvthumper: pushed changes for your review08:44
jtv(moving on to noodles' review)08:44
jtvnoodles775: pushed changes for your review08:47
jtvnoodles775: just pushed another tiny, tiny thing I missed in renaming that parameter.  Nothing you wouldn't expect.08:52
noodles775Yep, looking now.08:53
noodles775jtv: committer_id is much clearer, thanks. And test_pofile_new_translation_autoselect still passes fine for me locally.08:56
jtvnoodles775: maverick or lucid?08:57
jtvsame here :(08:59
jtvmay be a plugin08:59
jtvphone call…08:59
noodles775jtv, StevenK: the oops error just appeared on buildbot, so that one seems spurious.08:59
StevenKOooh, so it is real09:01
noodles775Real in the sense that it happens sometimes even on buildbot, yes.09:02
jtvnoodles775: by the way, sorry about the double-review hassle; I was knocking on doors and moving on when I got no reply.  Are things to your satisfaction now?09:17
noodles775jtv: no problem! Yep - I only reviewed the code as per my comment, but I'm happy with that (after your variable change).09:18
jtvnoodles775: thanks—then I'll land on devel09:18
jmlno one reviewed my branch yesterday09:39
jtvjml: as it happens, I'm OCR.  Putting out some fires today, which is why I haven't gotten around to the backlog, but I could do it now.09:39
jmljtv, that'd be great, thanks.09:39
=== jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: jml || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jtvjml: you're turning into a Hindu god.09:40
jmljtv, oh?09:41
jtvLots of creative destruction and, evidently, more than the normal quota of hands to type with.09:41
jtvjml: who is "we" in this narrative by the way?09:42
jtvRoyal "we"?09:42
jmljtv, bigjools & I09:43
jmljtv, and Shiva09:43
jtvjml: …and this is why the comma is a bad way to indicate that you're addressing someone directly in IRC.  :)09:43
jmljtv, I hate it the comma autocomplete09:43
jmljtv, but xchat doesn't give me a control to change it09:44
jtvah!  there are clients that do that?09:44
jtva mild bowel discomfort be upon them09:44
jtvjml: diff line 41 is weird: "Similarly, but"09:45
jmljtv, oh yeah. I guess "But" will do09:46
jtvjml: I'm unable to find the connection between the narrative and the test there, but that's probably just me.  If you assume me they match, that's good enough for me—as long as I get you to expend some thought on the matter.09:48
jmljtv, they do match. the broader narrative is about checking the deb lines.09:49
jtvjml: I believe you blindly.09:50
jmljtv, I don't think archive-dependencies.txt is a well-written document, and would rather (one day) move all of those to unit tests.09:50
jtvI'm glad you're making the effort.09:50
jtvjml: diff line 698 would read even better IMHO if you switched conditions around to read "... and not ..."09:56
jmljtv, good call09:57
jtvjml: also, I see some XXX comments that lack date, name, and bug reference.09:58
jmljtv, yeah, I'll quickly review those now.09:59
jtvjml: a bit scary that the OkSlave previously did almost nothing but then suddenly Popen'ed in resume()…10:03
jmljtv, yes, it is.10:05
jmljtv, lots of this stuff is scary10:05
jmljtv, maybe in a branch today or tomorrow we'll actually kill all of those mocks and do the mocking at the xmlrpclib layer.10:06
jtvthat sounds like a cool idea… so much of it is no-brainer replication anyway.10:06
jtvThe call log is quite close to what FakeMethod does, even.10:06
jmlyeah, but we are selective about what we append10:06
jmlthe whole thing is really close to niemeyer's mocker10:07
* jtv sobs quietly into a handkerchief, then composes himself10:07
jmlwhich I'm not sure why we don't use.10:07
jtvNot in this branch though!10:07
jtvBut yes, if that fits, by all means do it.10:07
jtvAh, a meaningful docstring on a test case.  The astrologists always said it would happen, but I didn't expect to see it in my lifetime.10:09
jmljtv, fwiw, Michael Foord, who was recently hired by Canonical, also has a popular mocking library :)10:09
jmlsomething like that :)10:09
jtvjml: why does TestBinaryBuildPackageBehavior.setUp invoke its super, create a factory, and log in?  Doesn't TestCaseWithFactory provide all that?10:10
jmljtv, TCWF does indeed provide all of that, but we are subclassing Twisted's TestCase, since we need the Deferred support10:11
jmljtv, and multi inheritance for TestCases is a nightmare.10:11
jtvjml: I only see it being derived from unittest.TestCase.10:11
jmljtv, "from twisted.trial import unittest"10:11
jtvthat is obscene10:12
jmljtv, we all make mistakes in our youth that we later come to regret10:12
jmljtv, one of mine was calling the main trial module 'unittest'10:12
jtvjml: was it legal in the country you grew up in?10:12
jtvah wait, I'm thinking of drugs there.10:13
jtvDifferent mistake.10:13
jmljtv, yes, but socially frowned upon.10:13
jtvGood, good.10:13
jtvHow about, in this special case, importing twisted and qualifying as twisted.trial.unittest.TestCase?10:13
jmljtv, well, we often do "from twisted.trial.unittest import TestCase as TrialTestCase"...10:14
jtvthat'd work too10:14
jmljtv, but I didn't do that here because it adds TrialTestCase to the test suite10:15
jml(also my fault)10:15
jtvCan you "from twisted.trial import unittest as twisted_unittest"?10:15
jmljtv, yeah, I'll do something like that.10:16
jtvThanks.  I sorely need a break; brb10:16
jtvjml: back from break… have you considered taking twisted out of the test altogether?10:27
jmljtv, no.10:27
jtvthe obvious follow-up: would it be worth considering?10:27
jtvI did that in one of the slave tests.10:28
jmljtv, ok, sorry, what I meant was that I considered and rejected it...10:28
jtvI forget how, sadly.10:28
jtvAh :)10:28
jmljtv, we're changing the BuilderSlave to have a Twisted API10:28
jmljtv, so that the buildmaster can be properly written10:28
jmljtv, that API change is going to bubble up and affect all of the *Behavior objects10:29
jtvAnd, I suppose, make it far too risky to try and hack your way out of twisted just for the purpose of this test.10:29
jmljtv, that's pretty much what's driving this work10:29
jmljtv, indeed.10:30
jmljtv, we *might* be able to do something tricky later on10:30
jmljtv, but until things are stabilized, I'd rather not.10:30
jtvBut Knuth's Law.  Got it.10:30
jmlYes. That one :)10:30
jml(as an example, lp.codehosting.vfs has code that works with either a synchronous xmlrpc client or an asynchronous one).10:31
jtvIn ll. 984—992 of the diff, would it perhaps be clearer to use a list comprehension, perhaps even without the nested function definition?10:33
jtvexpected = [10:33
jtv    ['cacheFile', 'sendFileToSlave', ('ensurepresent', url, '', '')]10:33
jtv    for url in [chroot.http_url] + extra_urls]10:33
jtvOh, you're extending, not appending.10:34
jmljtv, I'm open for ideas on making that code clearer.10:34
jmljtv, I had to switch to Haskell mode to be mentally capable of solving the problem10:35
jtvGood start.10:35
jtvjml: personally, I've come off the idea of building these great clever assertion methods in tests.  I do like the general approach, but find it better to return something I can compare in a single comparison directly in the test.10:38
jtvMakes failures much easier to deal with.10:38
jmljtv, certainly that would be nice10:39
jmljtv, the current API we're testing is a bit of a blunt instrument. I can't think of anything less bad than the assertion we have now.10:39
jtvjml: I think this might be clearer… http://paste.ubuntu.com/498340/10:41
jmljtv, you mean, a) change the function to a list comp; b) change the assertion method into a helper that builds the object to compare against?10:43
jtvjml: exactly10:43
jmlok. I'm willing to give that a try.10:43
jtv(Of course not all of the function will fit into a list comprehension, but forming a list would be the obvious core of it)10:43
jtvjml: I'd also extract the dict into a variable, which will break down the structure a bit and also allow the contents to follow our syntax guidelines.10:44
jmlhow do they break from our syntax guidelines?10:45
jtvsyntax guidelines say dictionary = {10:45
jtv    'item1': 1,10:45
jtv    'item2': 2,10:46
jtv    }10:46
jmloh right.10:46
jtvahhh, looking at the calls, I see why you have the assertion method.  It must be fun debugging this stuff.10:50
jmljtv, the test failures are relatively easy to grok.10:51
jtvbtw I guess twisted tests are expected to return a deferred, or watchumacallit?10:51
jmljtv, but, well, big fat APIs that do a lot of stuff are always hard to test.10:51
jmljtv, yes, they are10:51
jmljtv, full story: if a trial test returns a deferred, trial will wait for that deferred to fire before moving on to the next test.10:52
jmljtv, that facility is necessary when dealing w/ async code10:52
jtvjml: that sort of suggests itself… but makes the naming similarity to unittest all the more unfortunate10:52
jmljtv, in most ways it's quite similar to unittest10:53
jtvIt's good to follow the same patterns, but awkward to have a different call model in such a close semblance of unittest10:54
jtvjml: in ll. 1073—1077 of the diff, it may be easier to construct the expected error string outside of the assertion.10:56
jtvI wish these tests could be shorter, but see no very reasonable way to do it.10:57
jmlme neither.10:57
jmlI'm hoping that as we push deeper into this stuff something will present itself10:57
jtvYou could extract the chroot-setting part into a helper.  Bit ugly with the commit in the middle (why not transaction.commit, by the way?) but would extract some unhelpful text from the tests.10:59
jtvI suspect the setup of builder and slave could also be extracted.11:00
jmltheir construction is parametrized differently in different tests, so I'm a little bit wary of prematurely refactoring.11:02
jmlnot transaction.commit() because zopeless.11:02
jmlmaybe that doesn't matter these days11:03
jtvI don't think it does11:03
jmlIn that case, layer.txn ought to be hidden.11:03
jtvI'm not sure but I think layer.txn == transaction11:04
jtvjml: the archive and the build are parameterized differently, but the builder and slave..?11:05
jmljtv, parametrized by the archive, arch series, virtualized or not.11:06
jtvjml: of course there's nothing stopping a helper from accepting similar pameters11:06
jmlbut then when we add a new test we'll probably need to change the helper11:07
jtvit's you call though; the question is how relevant it is to the test11:07
jmlI'd like to leave it for now.11:07
jmlwhere are we at more generally w/ the review? bigjools is doing some stuff that I'd like to stick my nose into.11:07
jtvjml: I think we're through most of the code you added; after that I'll make one more brief pass to look for lost coverage but I don't expect to find anything.11:08
jmljtv, cool. thanks.11:09
jtvjml: I suppose the bit in lines 63—99 is really just unnecessarily duplicating Librarian work?11:11
jtvjml: I'm saturated, so not much use for me to look through this further.  I'll have to trust that the coverage matches, or if not, that (1) people will be afraid to touch the code anyway or (2) any failures will cause widespread panic and so will be instantly fixed and tested in a modern way.11:15
jmljtv, understood11:16
jtvjml: got a revision for me?  I'm EOD'ing as far as all else is concerned.11:16
jmljtv, the 63-99 is already tested in buildmaster/model/tests/test_builder.py11:16
jmljtv, http://pastebin.ubuntu.com/498363/11:16
jmljtv, I had to keep the assert helper because the cookie is only created after startJob is called.11:17
jtvjml: understood, I think… still think you've made an improvement there.11:18
jmljtv, thanks.11:18
jtvBrings the interesting part of a failure one level closer to where the reader looks for it.11:19
jtvIn the traceback, I mean.11:19
jtvjml: I've approved.11:26
jtvOn call: - || Reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews11:26
jtvI meant…11:27
=== jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jmljtv, thanks.11:34
=== mrevell is now known as mrevell-lunch
deryckadeuring, hi, can I get a review from you? (he says, picking a bugs guy at random.)13:37
adeuringderyck: sure13:38
deryckadeuring, thanks!  https://code.edge.launchpad.net/~deryck/launchpad/displayname-private-team-oops-634847/+merge/3619913:38
=== sinzui changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== jcsackett changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [sinzui, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jcsackettMP for me: https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-blueprints-service-597738-2/+merge/3624213:48
adeuringderyck: r=me13:54
deryckadeuring, awesome, thanks!13:55
marsStevenK, still online?14:29
StevenKmars: I am14:40
marsStevenK, heya, did you get to any of the reviews that I left in the queue last night?14:41
StevenKmars: You know, I forgot I was OCR today. :-(14:41
marsStevenK, no problem, everyone does that from time to time14:42
marsStevenK, if you would like, there are two reviews in the channel queue, and one in +activereviews.  But I realize it is late there, well past your EOD14:42
StevenKmars: Yup, I don't think I can think straight enough :-)14:43
marsStevenK, next week then.  leonardr might be OCR, but we can do the same thing again, and save up some reviews for you.  There are usually a few at the end of the day here14:44
marsso it doesn't impact the developers if leonardr and I save them for you14:44
sinzuiEdwinGrubbs, ping15:15
EdwinGrubbssinzui: hi15:16
sinzuiEdwinGrubbs,  can you review the two sql scripts that Chex and I tested on staging yesterday to clean up deactivated/merged user data? We need another engineer to agree they are sane: http://pastebin.ubuntu.com/498514/ and http://pastebin.ubuntu.com/498518/15:17
sinzuiEdwinGrubbs, Staging ran fine, and the UI looked great after we ran them15:18
EdwinGrubbssinzui: wouldn't it be a good idea to check whether Person.merged is not null instead of just trusting the name?15:32
sinzuiEdwin I think that would be a good idea15:33
EdwinGrubbssinzui: I don't see any other issues. However, it would be nice if a backup of those tables could be taken just in case some user needs to be reverted however unlikely that is.15:45
sinzuiEdwinGrubbs, since we tested on staging. without any issues. I think I should revise the script, maybe combine the deactivated/merged rule so that it is one script. Then test it on staging next week with fresh data15:46
EdwinGrubbssinzui: will removing the team memberships have an adverse affect on the non-human user ~katie, who is suspended and a celebrity in the code.15:52
sinzuiOh :( I bet it will15:53
sinzuiI need to think about that15:53
sinzuiEdwin do you think is it safe to remove suspended users from the invited and proposed states, but ignore those that are members? I think it is better to leave some cruft behind and run the parts we trust to work15:58
rockstarabentley, could I get a review of https://code.edge.launchpad.net/~rockstar/launchpad/build-farm-job-constraint/+merge/3621916:03
rockstar(I was asked to leave it for StevenK so that he could get review experience, but he didn't touch it yesterday)16:03
abentleyrockstar, r=me16:04
rockstarabentley, thanks.16:06
=== gary_poster is now known as gary-lunch
deryckadeuring, hi.  So a couple minor thoughts about your branch....16:41
deryckadeuring, do you really need "We’ve" in place of the plain "We've" in HTML?16:41
adeuringderyck: I simply copied that from the regular error page16:42
adeuringbut I can change it to a simple "'"16:42
=== EdwinGrubbs changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || Reviewing: - || queue: [sinzui, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
deryckadeuring, yeah, let's do that.   Unless there's some compelling reason to encode that character.16:43
deryckadeuring, and then I tend to prefer using ellipses to hide more of that error mesaage in the page test.  In case the template changes it doesn't make the test fragile.16:44
deryckadeuring, but that's up to you.  Otherwise, r=me.16:44
adeuringderyck: right using more "..." makes sense16:45
adeuringderyck: and thanks!16:45
deryckadeuring, np16:46
deryckthanks for the fix.16:46
=== benji is now known as benji-lunch
=== EdwinGrubbs is now known as Edwin-lunch
=== Ursinha is now known as Ursinha-lunch
=== gary-lunch is now known as gary_poster
=== benji-lunch is now known as benji
=== deryck is now known as deryck[lunch]
=== Ursinha-lunch is now known as Ursinha
=== deryck[lunch] is now known as deryck
=== Edwin-lunch is now known as EdwinGrubbs
leonardrEdwinGrubbs, who can do a ui review? i haven't needed one since we instituted the process, but i need one now19:53
EdwinGrubbsleonardr: I can do it, but I'm not graduated, so you will have to get two reviews. noodles has graduated, and he is oncall tomorrow morning.19:56
leonardrok, cool19:56
leonardri'll get something ready19:56
abentleyEdwinGrubbs, could you please review https://code.launchpad.net/~abentley/launchpad/fix-groupby/+merge/36360 ?20:13
EdwinGrubbsabentley: sure, I'll let you jump the queue since the branch is so small.20:14
abentleyEdwinGrubbs, thanks.20:15
jcsackettEdwinGrubbs: realized i posted this before you came on call: https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-blueprints-service-597738-2/+merge/36242; it's the MP i already put myself in the queue for, when you have time.20:20
EdwinGrubbsjcsackett: I should get to that in about an hour.20:21
jcsackettEdwinGrubbs: fantastic. thanks. :-)20:21
leonardredwingrubbs: https://code.edge.launchpad.net/~leonardr/launchpad/rename-grant-permissions/+merge/3636320:44
leonardrif you can't get to it, no big deal20:44
EdwinGrubbswe'll see20:44
EdwinGrubbsabentley: r=me21:02
abentleyEdwinGrubbs, thanks.21:02
=== EdwinGrubbs changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || Reviewing: sinzui || queue: [jcsackett, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
rockstarabentley, could you take a look at: https://code.edge.launchpad.net/~rockstar/launchpad/recipe-too-new/+merge/3637921:40
abentleyrockstar, you need to do monkeypatches in a try/finally block; otherwise, they can leak.21:42
abentleyrockstar, or you could use a with statement.21:42
abentleyrockstar, the with statement gives you a little code reuse.21:43
rockstarabentley, so, you're suggesting creating a contextmanager that does the monkey patch?21:44
abentleyrockstar, yeah, I think that's the cleanest way.21:44
rockstarabentley, okay.21:44
abentleyr=me with that change.21:48
EdwinGrubbssinzui: review sent21:48
rockstarabentley, where should that context manager live?  lp.code.testing doesn't exist, but that makes the most sense.21:50
abentleyrockstar, we use lp.code.tests.helpers for that kind of thing.21:50
rockstarAh, even better.21:50
sinzuithanks edwin21:51
rockstarabentley, a cursory review of http://bazaar.launchpad.net/~rockstar/launchpad/recipe-too-new/revision/11608 please?22:07
=== Ursinha is now known as Ursinha-afk
abentleyrockstar, you should do the assignment to RecipeParser.NEWEST_VERSION before the try.  The only thing in the try should be the yield.22:10
rockstarabentley, really?  Is there some extra logic in the context manager then?22:10
abentleyrockstar, in this case, it doesn't matter, because the teardown doesn't require the setup to have succeeded.22:10
rockstarabentley, ah, okay.22:10
abentleyrockstar, no.  This is true of all try/finally blocks.22:10
rockstarabentley, yeah, I hadn't really thought that through.22:11
EdwinGrubbsjcsackett: r=me22:14
jcsackettEdwinGrubbs: thanks.22:14
=== salgado is now known as salgado-afk
EdwinGrubbsleonardr: review sent22:51
=== EdwinGrubbs changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
leonardredwin, thanks22:58
=== Ursinha-afk is now known as Ursinha
=== EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews

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