[00:42] <rockstar> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/recipe-oopses/+merge/27313
[02:21] <rockstar> thumper, https://code.edge.launchpad.net/~rockstar/launchpad/fix-branch-requestMirror/+merge/27317
[02:21] <thumper> rockstar: hey
[02:21] <thumper> rockstar: looking at the oopses one
[02:24] <rockstar> thumper, cool.
[03:52] <rockstar> thumper, ping
[03:53] <rockstar> thumper, re: "Is it right to hard code the formats?" we talked about this same issue when I first worked on BranchUpgradeJob.  We decided it wasn't a big deal, and especially not since bzr won't be changing its default format until 3.0.
[03:58] <thumper> rockstar: I just think it would be better to get the formats out of the branch itself
[03:58] <thumper> rockstar: in that particular situation
[03:59] <rockstar> thumper, sure.  I can do that.
[03:59] <thumper> rockstar: thansk
[04:02] <rockstar> thumper, can you look at the other one as well?
[04:02] <thumper> rockstar: I did
[04:02] <thumper> I even commented
[04:15] <rockstar> thumper, I mean this one: https://code.edge.launchpad.net/~rockstar/launchpad/recipe-oopses/+merge/27313
[04:16] <thumper> ?!?
[04:16] <thumper> I did
[04:16] <thumper> damn, wtf?
[04:17]  * thumper tries again
[04:17] <thumper> rockstar: look now
[04:20] <thumper> rockstar: I have one for you if you want
[04:23] <rockstar> thumper, sure, I can look at it.
[04:24] <thumper> rockstar: did you see the buildbot failure?
[04:24] <thumper> rockstar: it is a windmill failure
[04:24] <rockstar> thumper, no, I haven't seen it.
[04:26] <rockstar> thumper, how do I take bzr branch format and get the corresponding BranchFormat enum?
[04:26]  * rockstar forgets...
[04:26] <thumper> rockstar: take a look in the codehosting ssh server
[04:26] <thumper> rockstar: I don't remember
[04:26] <thumper> but that is the other location it is done
[04:33] <thumper> rockstar: https://code.edge.launchpad.net/~thumper/launchpad/branch-index-slowness/+merge/27318
[04:37] <thumper> rockstar: I've somehow managed to change my gnome window decoration to something that looks like it is out of the 90s
[04:37] <thumper> it is like the icon set changed
[04:37] <rockstar> thumper, log out and log back in.
[04:37] <rockstar> The gnome config daemon probably died...
[04:37] <rockstar> (happens to me all the time)
[04:37] <thumper> eh?
[04:38] <rockstar> It's hella annoying.
[04:38] <thumper> ah, yeah
[04:38] <thumper> that's why I left kubuntu
[04:38] <thumper> well, left
[04:38] <thumper> well, trying gnome for a bit
[04:39] <rockstar> thumper, Lucid is not the best gnome foot forward...  :/
[05:02] <rockstar> thumper, line 150 of your diff, why not use isinstance?
[05:03] <thumper> rockstar: because I didn't think of it :)
[05:03] <rockstar> thumper, okay.  Can you remedy that?
[05:03] <thumper> yep
[05:04] <rockstar> thumper, do you have some data to go with this branch that might be good to go to the list?
[05:04] <thumper> rockstar: what are you asking exactly
[05:06] <rockstar> thumper, I think query count numbers along with the changes in this branch would be good to post to the list.
[05:07] <thumper> rockstar: I'll be writing something up, yes
[05:07] <rockstar> thumper, also, there are new changes to look at on this review: https://code.edge.launchpad.net/~rockstar/launchpad/fix-branch-requestMirror/+merge/27317
[05:09] <thumper> rockstar: you don't close the branch
[05:10] <thumper> rockstar: you'd make me very happy if you created a context manager
[05:10] <thumper> rockstar: and went "with BzrBranch.open_from_transport(source_branch_transport) as branch:" ...
[05:11] <rockstar> thumper, I can move ForbiddenInstruction to lp.code.errors.  I think hardcoding the instruction isn't that bad, since there are only three actual instructions in the first place.  Thirdly, I can't do the catch of the exception in the validate because the exception isn't raised until we actually try and make the changes.
[05:11] <rockstar> abentley and I talked about the third option, and we both came to the conclusion that it's the best way to do it.
[05:11] <thumper> rockstar: isn't there a validator for the recipe?
[05:11] <thumper> rockstar: there are only three instructions now, what about later?
[05:12] <thumper> rockstar: if the information is in the exception (and it should be) then it shouldn't be hard to use right?
[05:12] <rockstar> thumper, I think that would make the code more counterintuitive than it needs to be.
[05:12]  * thumper has to go move a sofa
[05:12] <thumper> why?
[05:13] <abentley> thumper, it follows Easier to Ask Forgiveness Than Permission.
[05:17] <thumper> which is against the zope way of doing views
[05:19] <thumper> however...
[05:19] <thumper> I think we can let this go here for now
[05:19] <thumper> as we have a precident
[05:21] <abentley> thumper, if EAFP is against Zope conventions, that's another example of our architecture/conventions not encouraging optimal code, both from a speed and maintenance perspective.
[05:22] <thumper> abentley: it is what the validate method is for
[05:22] <abentley> thumper, preventing speed and maintainability?
[05:22] <thumper> abentley: I _think_ it is to offer a single place of validation for potentially many actions
[05:24] <abentley> thumper, if that is the single place for validation, then we can't have validation in the model, and that leads to madness.
[05:25] <thumper> abentley: true
[05:25] <thumper> We are no longer using views how zope intended I think
[05:25] <thumper> given our API exposure of the models
[05:26] <abentley> thumper, well...
[05:27] <abentley> thumper, I think validation of the input has a place.  e.g. if x is supposed to be an INT, make sure it's an INT.
[05:28] <abentley> thumper, but I don't think the views should attempt to replicate the model rules.  It means you have to keep two pieces of code in sync.
[05:28] <thumper> yeah, that makes sense
[05:29] <abentley> thumper, until rockstar pointed out that actions could call setFieldError, I assumed we had no choice.
[05:38] <rockstar> thumper, on further inspection, it looks like we can't get to the actual message.  bzr-builder would need a patch before we could.
[05:38] <thumper> rockstar: what about the command?
[05:40] <rockstar> thumper, yeah, the command actually doesn't have any information about what the actual command is.
[05:40] <thumper> rockstar: ok then.  Can you file a bug and XXX the callsite
[05:40] <thumper> ?
[05:41] <rockstar> thumper, ack.
[05:41] <rockstar> thumper, I think we can probably even patch it fairly simply, now that I'm looking at it.
[06:07] <rockstar> thumper, this one is updated: https://code.edge.launchpad.net/~rockstar/launchpad/recipe-oopses/+merge/27313
[06:07] <rockstar> thumper, I'm not sure what the context manager gives us.
[06:08] <thumper> closing the branch and nicer code
[06:08] <thumper> also, this has conflicts now
[06:09] <rockstar> thumper, no, I resolved those, wait for the diff.
[06:09]  * thumper waits
[06:09] <rockstar> thumper, where else are we "closing" a branch?
[06:09] <thumper> rockstar: don't know, but if you open something, it is good manners to close it :)
[06:09] <rockstar> There's not even a "close" method.
[06:09] <thumper> really?
[06:10] <rockstar> thumper, I don't see one.
[06:10] <rockstar> thumper, this is why I ask where else we're doing it.
[06:10]  * thumper asked on #bzr
[06:15] <rockstar> thumper, I don't see anywhere where we close a branch.  Even BzrSync.syncBranchAndClose just unlocks the branch.
[06:37] <thumper> rockstar: forget about the context manager then
[06:38] <rockstar> thumper, I can has approve then?
[06:38] <thumper> aye
[06:38] <rockstar> thumper, thanky
[06:39] <rockstar> thumper, talk to you in week 4.
[06:40] <thumper> rockstar: have a good break
[14:37] <bac> hello adeuring
[14:38] <adeuring> hi bac
[14:38] <bac> adeuring: just letting you know i'm on call now too
[14:38] <adeuring> bac: thanks; seems to be a quiet day :)
[14:38] <bac> cool
[14:53] <bac> adeuring: did you see that sinzui stealthily added branches to the queue?  you want to split them up?
[14:53] <adeuring> bac: whoops... no
[14:53] <bac> i'm not sure why he always flies under the radar like that.  very unproductive
[14:54] <adeuring> OK, I'll take https://code.edge.launchpad.net/~sinzui/launchpad/front-page-footer-0/+merge/27311
[14:54] <bac> ok
[15:12] <bac> hi sinzui, question about your test for PRF changes
[15:12] <bac> well, actually about the dispay of latest downloads that changed in that branch
[15:12] <bac> sinzui: you have a new test showing obsolete series are ignored
[15:13] <bac> before setting s4 to obsolete, the latest with downloads was 3.3, which is a part of s3.  so even if s4 is marked obsolete shouldn't 3.3 still display?
[15:15] <sinzui> bac: I think I demonstrated that in the test.
[15:16]  * sinzui looks
[15:17] <sinzui> bac, waaa..
[15:18] <sinzui> bac: I am going to run that test. It looks wrong and I think I did say 3.3 was the latest. I may have backed that out of the test
[15:18] <bac> sinzui: ok.  it just looks funny to the gentle reader.
[15:19] <sinzui> :(
[15:20] <sinzui> bac, I did backout my change
[15:20] <sinzui> well, ctrl-z
[15:20] <sinzui> ah
[15:20] <sinzui> Wow
[15:20] <bac> sinzui: the test passes for me, which makes me confused
[15:20] <sinzui> I really messed that up. current is 4.3, and after obsolescence it is 3.3
[15:21] <sinzui> no way,
[15:21] <sinzui> bac, http://pastebin.ubuntu.com/448268/
[15:22] <sinzui> I will make the change I intended to make. What I see in the review is the test I wrote before I changed the code and saw I also misread the test
[15:23] <bac> sinzui: oh you're right, it does fail for me
[15:23] <bac> sinzui: you didn't include that test in the "test" line of your MP so i hadn't actually run it
[15:24] <sinzui> It was just after midnight I screwed up.
[15:24] <sinzui> bac: http://pastebin.ubuntu.com/448271/
[15:26] <bac> sinzui: see, curtis, turning into a pumpkin is just a metaphor.  you really should stop work after midnight.
[15:47] <adeuring> sinzui: r=me; two nitpicks
[15:47] <sinzui> adeuring, thanks
[16:01] <bac> sinzui: r=bac, with a couple of changes and one absurd request
[16:02] <sinzui> thank bac
[16:31] <sinzui> bac ping
[16:32] <bac> hi sinzui
[16:32] <sinzui> about your points.
[16:32] <sinzui> 1. I too hate glob
[16:32] <sinzui> and it is not clear what uses it
[16:33] <bac> wellispecificallyhatedthelackofseparatorsinthename
[16:33] <sinzui> We can start by making  good api name. maybe release_finder_url_pattern?
[16:37] <bac> sinzui: hey that's much better!
[16:37] <bac> cause, you know what?  it *is* an URL pattern
[16:38] <bac> sinzui: fwiw i did exercise it via lplib and got validation errors when i tried just using 'cow', etc
[16:38] <sinzui> yep
[16:39] <sinzui> I want the name to state what it is and for. One day I will make the release finder public and easy to understand
[16:39] <bac> nice
[16:39] <sinzui> bac: may I change the name in the api then?
[16:39] <bac> sinzui: i think that's a swell idea
[16:40] <sinzui> I am not interested in adding an lplib test for this. I added it because I personally need this to see the damage that has been done on edge. No one cares about this as a feature until users know it is a feature
[18:13] <gary_poster> adeuring or bac: up for a review of a very small chance to our meta dependencies?  https://code.edge.launchpad.net/~gary/meta-lp-deps/geoiplite/+merge/27381
[18:14] <adeuring> gary_poster: I'll look
[18:15] <gary_poster> Thank you adeuring. Note that there is a pre-existing dependency on geoip-data | geoip-data-city-lite fro launchpad-dependencies.  I'm just trying to say that devs (and people expecting to run tests)really need the lite version
[18:15] <gary_poster> for
[18:15] <adeuring> gary_poster: ok, r=me
[18:16] <gary_poster> thank you adeuring
[18:20] <maxb> gary_poster: So you're implying that launchpad itself can run with geoip-data, but the tests need -city-lite?
[18:20] <gary_poster> maxb: yeah.  I just committed it and am pushing it.  Is that OK by you?
[18:21] <maxb> if that's the case, fine. It just feels a bit perverse that the tests need one thing and production needs another
[18:21] <gary_poster> I agree
[18:21] <gary_poster> I think it is about licensing issues, and open-source issues
[18:35] <bac> sinzui: you are in the topic queue but i think that is a mistake, right?  i see nothing on activereviews
[18:36] <sinzui> bac: yes I think I am done
[19:57] <EdwinGrubbs> bac: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-590840-getMembershipInformation-timeout-2/+merge/27394
[19:57] <bac> i will
[20:02] <bac> EdwinGrubbs: in the MP you talk about "testing both of the unioned queries".  i don't see in your changes where that is addressed.  could you clarify?
[20:04] <EdwinGrubbs> bac: sorry, that was unclear. registry/doc/mailinglist-subscriptions.txt already tests the team_members query in getSenderAddresses(). I added the tests in message-holds.txt to test the approved_posters query which gets unioned to to team_members in getSenderAddresses(team_names).
[20:05] <EdwinGrubbs> bac: it's a little confusing that I'm adding tests for getSenderAddresses(team_names), but the branch also modifies the other getSenderAddresses().
[20:10] <bac> EdwinGrubbs: ok.  that makes sense.  the branch proper looks fine...i was just confused.
[20:11] <bac> EdwinGrubbs: finally, i assume there was no real benefit to returning a generator instead of the storm resultset as you do now.
[20:12] <bac> oh, i see they were filtering the results too, which you don't have to do now
[20:13] <EdwinGrubbs> bac: right, the generator is just overhead
[21:48] <bac> sinzui: is your other branch going to make it today?
[21:48] <sinzui> No
[21:49] <sinzui> I suck
[21:49] <bac> doh
[21:49] <bac> sinzui: well i will look at it over the weekend if you want
[21:49] <sinzui> No rush
[21:49] <sinzui> I will work on lint over the weekend though
[21:49] <bac> schweet
[21:55] <sinzui> bac: I have a 19503 line branch that removes glob imports from our tree.
[21:55] <bac> really?
[21:55] <sinzui> I fear the review would kill
[21:55] <bac> how many 800 line branches would that be?
[21:55] <sinzui> 4
[21:56] <sinzui> But this is a mechanical branch
[21:56] <bac> sinzui: i can work on it this weekend
[21:56] <sinzui> I have fixed all but one call site
[21:57] <sinzui> c.l.__init__ has a lot in it still because removing all causes cyclic import problems
[21:58] <sinzui> bac: I want an rs to get this into a safe state, so that future branches can do the hard thinking work of decoupling the cyclic imports
[21:59] <bac> why don't you do a rs=sinzui then?  you know what you've done better than i do and that is perfectly legal
[22:00] <sinzui> You have a good point.
[22:00] <sinzui> gary talked me off the ledge this week so the scope is safe