[00:42] thumper, https://code.edge.launchpad.net/~rockstar/launchpad/recipe-oopses/+merge/27313 [02:21] thumper, https://code.edge.launchpad.net/~rockstar/launchpad/fix-branch-requestMirror/+merge/27317 [02:21] rockstar: hey [02:21] rockstar: looking at the oopses one [02:24] thumper, cool. [03:52] thumper, ping [03:53] 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] rockstar: I just think it would be better to get the formats out of the branch itself [03:58] rockstar: in that particular situation [03:59] thumper, sure. I can do that. [03:59] rockstar: thansk [04:02] thumper, can you look at the other one as well? [04:02] rockstar: I did [04:02] I even commented [04:15] thumper, I mean this one: https://code.edge.launchpad.net/~rockstar/launchpad/recipe-oopses/+merge/27313 [04:16] ?!? [04:16] I did [04:16] damn, wtf? [04:17] * thumper tries again [04:17] rockstar: look now [04:20] rockstar: I have one for you if you want [04:23] thumper, sure, I can look at it. [04:24] rockstar: did you see the buildbot failure? [04:24] rockstar: it is a windmill failure [04:24] thumper, no, I haven't seen it. [04:26] thumper, how do I take bzr branch format and get the corresponding BranchFormat enum? [04:26] * rockstar forgets... [04:26] rockstar: take a look in the codehosting ssh server [04:26] rockstar: I don't remember [04:26] but that is the other location it is done [04:33] rockstar: https://code.edge.launchpad.net/~thumper/launchpad/branch-index-slowness/+merge/27318 [04:37] rockstar: I've somehow managed to change my gnome window decoration to something that looks like it is out of the 90s [04:37] it is like the icon set changed [04:37] thumper, log out and log back in. [04:37] The gnome config daemon probably died... [04:37] (happens to me all the time) [04:37] eh? [04:38] It's hella annoying. [04:38] ah, yeah [04:38] that's why I left kubuntu [04:38] well, left [04:38] well, trying gnome for a bit [04:39] thumper, Lucid is not the best gnome foot forward... :/ [05:02] thumper, line 150 of your diff, why not use isinstance? [05:03] rockstar: because I didn't think of it :) [05:03] thumper, okay. Can you remedy that? [05:03] yep [05:04] thumper, do you have some data to go with this branch that might be good to go to the list? [05:04] rockstar: what are you asking exactly [05:06] thumper, I think query count numbers along with the changes in this branch would be good to post to the list. [05:07] rockstar: I'll be writing something up, yes [05:07] 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] rockstar: you don't close the branch [05:10] rockstar: you'd make me very happy if you created a context manager [05:10] rockstar: and went "with BzrBranch.open_from_transport(source_branch_transport) as branch:" ... [05:11] 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] 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] rockstar: isn't there a validator for the recipe? [05:11] rockstar: there are only three instructions now, what about later? [05:12] rockstar: if the information is in the exception (and it should be) then it shouldn't be hard to use right? [05:12] 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] why? [05:13] thumper, it follows Easier to Ask Forgiveness Than Permission. [05:17] which is against the zope way of doing views [05:19] however... [05:19] I think we can let this go here for now [05:19] as we have a precident [05:21] 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] abentley: it is what the validate method is for [05:22] thumper, preventing speed and maintainability? [05:22] abentley: I _think_ it is to offer a single place of validation for potentially many actions [05:24] 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] abentley: true [05:25] We are no longer using views how zope intended I think [05:25] given our API exposure of the models [05:26] thumper, well... [05:27] 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] 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] yeah, that makes sense [05:29] thumper, until rockstar pointed out that actions could call setFieldError, I assumed we had no choice. [05:38] 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] rockstar: what about the command? [05:40] thumper, yeah, the command actually doesn't have any information about what the actual command is. [05:40] rockstar: ok then. Can you file a bug and XXX the callsite [05:40] ? [05:41] thumper, ack. [05:41] thumper, I think we can probably even patch it fairly simply, now that I'm looking at it. [06:07] thumper, this one is updated: https://code.edge.launchpad.net/~rockstar/launchpad/recipe-oopses/+merge/27313 [06:07] thumper, I'm not sure what the context manager gives us. [06:08] closing the branch and nicer code [06:08] also, this has conflicts now [06:09] thumper, no, I resolved those, wait for the diff. [06:09] * thumper waits [06:09] thumper, where else are we "closing" a branch? [06:09] rockstar: don't know, but if you open something, it is good manners to close it :) [06:09] There's not even a "close" method. [06:09] really? [06:10] thumper, I don't see one. [06:10] thumper, this is why I ask where else we're doing it. [06:10] * thumper asked on #bzr [06:15] thumper, I don't see anywhere where we close a branch. Even BzrSync.syncBranchAndClose just unlocks the branch. [06:37] rockstar: forget about the context manager then [06:38] thumper, I can has approve then? [06:38] aye [06:38] thumper, thanky [06:39] thumper, talk to you in week 4. [06:40] rockstar: have a good break === adeuring changed the topic of #launchpad-reviews to: On Call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === sinzui changed the topic of #launchpad-reviews to: On Call: adeuring || reviewing: - || queue: [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === bac changed the topic of #launchpad-reviews to: On Call: adeuring,bac || reviewing: - || queue: [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [14:37] hello adeuring [14:38] hi bac [14:38] adeuring: just letting you know i'm on call now too [14:38] bac: thanks; seems to be a quiet day :) [14:38] cool [14:53] adeuring: did you see that sinzui stealthily added branches to the queue? you want to split them up? [14:53] bac: whoops... no [14:53] i'm not sure why he always flies under the radar like that. very unproductive [14:54] OK, I'll take https://code.edge.launchpad.net/~sinzui/launchpad/front-page-footer-0/+merge/27311 [14:54] ok === adeuring changed the topic of #launchpad-reviews to: On Call: adeuring,bac || reviewing: sinzui, - || queue: [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === adeuring changed the topic of #launchpad-reviews to: On Call: adeuring,bac || reviewing: sinzui, - || queue: [ sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [15:12] hi sinzui, question about your test for PRF changes [15:12] well, actually about the dispay of latest downloads that changed in that branch [15:12] sinzui: you have a new test showing obsolete series are ignored [15:13] 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] bac: I think I demonstrated that in the test. [15:16] * sinzui looks [15:17] bac, waaa.. [15:18] 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] sinzui: ok. it just looks funny to the gentle reader. [15:19] :( [15:20] bac, I did backout my change [15:20] well, ctrl-z [15:20] ah [15:20] Wow [15:20] sinzui: the test passes for me, which makes me confused [15:20] I really messed that up. current is 4.3, and after obsolescence it is 3.3 [15:21] no way, [15:21] bac, http://pastebin.ubuntu.com/448268/ [15:22] 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] sinzui: oh you're right, it does fail for me [15:23] sinzui: you didn't include that test in the "test" line of your MP so i hadn't actually run it [15:24] It was just after midnight I screwed up. [15:24] bac: http://pastebin.ubuntu.com/448271/ [15:26] sinzui: see, curtis, turning into a pumpkin is just a metaphor. you really should stop work after midnight. [15:47] sinzui: r=me; two nitpicks === adeuring changed the topic of #launchpad-reviews to: On Call: adeuring,bac || reviewing: -, - || queue: [ sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [15:47] adeuring, thanks [16:01] sinzui: r=bac, with a couple of changes and one absurd request === bac changed the topic of #launchpad-reviews to: On Call: adeuring,bac || reviewing: -, - || queue: [ -] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [16:02] thank bac [16:31] bac ping [16:32] hi sinzui [16:32] about your points. [16:32] 1. I too hate glob [16:32] and it is not clear what uses it [16:33] wellispecificallyhatedthelackofseparatorsinthename [16:33] We can start by making good api name. maybe release_finder_url_pattern? [16:37] sinzui: hey that's much better! [16:37] cause, you know what? it *is* an URL pattern [16:38] sinzui: fwiw i did exercise it via lplib and got validation errors when i tried just using 'cow', etc [16:38] yep [16:39] 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] nice [16:39] bac: may I change the name in the api then? [16:39] sinzui: i think that's a swell idea [16:40] 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 === deryck_ is now known as deryck === deryck is now known as deryck[lunch] [18:13] 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] gary_poster: I'll look [18:15] 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] for [18:15] gary_poster: ok, r=me [18:16] thank you adeuring [18:20] gary_poster: So you're implying that launchpad itself can run with geoip-data, but the tests need -city-lite? [18:20] maxb: yeah. I just committed it and am pushing it. Is that OK by you? [18:21] if that's the case, fine. It just feels a bit perverse that the tests need one thing and production needs another [18:21] I agree [18:21] I think it is about licensing issues, and open-source issues === deryck[lunch] is now known as deryck === adeuring changed the topic of #launchpad-reviews to: On Call: bac || reviewing: - || queue: [ sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [18:35] sinzui: you are in the topic queue but i think that is a mistake, right? i see nothing on activereviews [18:36] bac: yes I think I am done === bac changed the topic of #launchpad-reviews to: On Call: bac || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [19:57] bac: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-590840-getMembershipInformation-timeout-2/+merge/27394 [19:57] i will === bac changed the topic of #launchpad-reviews to: On Call: bac || reviewing: edwin || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [20:02] 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] 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] 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] EdwinGrubbs: ok. that makes sense. the branch proper looks fine...i was just confused. [20:11] EdwinGrubbs: finally, i assume there was no real benefit to returning a generator instead of the storm resultset as you do now. [20:12] oh, i see they were filtering the results too, which you don't have to do now [20:13] bac: right, the generator is just overhead [21:48] sinzui: is your other branch going to make it today? [21:48] No [21:49] I suck [21:49] doh [21:49] sinzui: well i will look at it over the weekend if you want [21:49] No rush [21:49] I will work on lint over the weekend though [21:49] schweet [21:55] bac: I have a 19503 line branch that removes glob imports from our tree. [21:55] really? [21:55] I fear the review would kill [21:55] how many 800 line branches would that be? [21:55] 4 [21:56] But this is a mechanical branch [21:56] sinzui: i can work on it this weekend [21:56] I have fixed all but one call site [21:57] c.l.__init__ has a lot in it still because removing all causes cyclic import problems [21:58] 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] 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] You have a good point. [22:00] gary talked me off the ledge this week so the scope is safe === matsubara is now known as matsubara-afk