| rockstar | thumper, https://code.edge.launchpad.net/~rockstar/launchpad/recipe-oopses/+merge/27313 | 00:42 |
|---|---|---|
| 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:21 |
| rockstar | thumper, cool. | 02:24 |
| rockstar | thumper, ping | 03:52 |
| 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:53 |
| 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:58 |
| rockstar | thumper, sure. I can do that. | 03:59 |
| thumper | rockstar: thansk | 03:59 |
| rockstar | thumper, can you look at the other one as well? | 04:02 |
| thumper | rockstar: I did | 04:02 |
| thumper | I even commented | 04:02 |
| rockstar | thumper, I mean this one: https://code.edge.launchpad.net/~rockstar/launchpad/recipe-oopses/+merge/27313 | 04:15 |
| thumper | ?!? | 04:16 |
| thumper | I did | 04:16 |
| thumper | damn, wtf? | 04:16 |
| * thumper tries again | 04:17 | |
| thumper | rockstar: look now | 04:17 |
| thumper | rockstar: I have one for you if you want | 04:20 |
| rockstar | thumper, sure, I can look at it. | 04:23 |
| 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:24 |
| 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:26 |
| thumper | rockstar: https://code.edge.launchpad.net/~thumper/launchpad/branch-index-slowness/+merge/27318 | 04:33 |
| 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:37 |
| 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:38 |
| rockstar | thumper, Lucid is not the best gnome foot forward... :/ | 04:39 |
| rockstar | thumper, line 150 of your diff, why not use isinstance? | 05:02 |
| thumper | rockstar: because I didn't think of it :) | 05:03 |
| rockstar | thumper, okay. Can you remedy that? | 05:03 |
| thumper | yep | 05:03 |
| 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:04 |
| rockstar | thumper, I think query count numbers along with the changes in this branch would be good to post to the list. | 05:06 |
| 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:07 |
| thumper | rockstar: you don't close the branch | 05:09 |
| 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:10 |
| 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:11 |
| 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:12 |
| abentley | thumper, it follows Easier to Ask Forgiveness Than Permission. | 05:13 |
| thumper | which is against the zope way of doing views | 05:17 |
| thumper | however... | 05:19 |
| thumper | I think we can let this go here for now | 05:19 |
| thumper | as we have a precident | 05:19 |
| 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:21 |
| 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:22 |
| 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:24 |
| 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:25 |
| abentley | thumper, well... | 05:26 |
| 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:27 |
| 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:28 |
| abentley | thumper, until rockstar pointed out that actions could call setFieldError, I assumed we had no choice. | 05:29 |
| 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:38 |
| 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:40 |
| rockstar | thumper, ack. | 05:41 |
| rockstar | thumper, I think we can probably even patch it fairly simply, now that I'm looking at it. | 05:41 |
| 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:07 |
| thumper | closing the branch and nicer code | 06:08 |
| thumper | also, this has conflicts now | 06:08 |
| 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:09 |
| 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:10 | |
| rockstar | thumper, I don't see anywhere where we close a branch. Even BzrSync.syncBranchAndClose just unlocks the branch. | 06:15 |
| thumper | rockstar: forget about the context manager then | 06:37 |
| rockstar | thumper, I can has approve then? | 06:38 |
| thumper | aye | 06:38 |
| rockstar | thumper, thanky | 06:38 |
| rockstar | thumper, talk to you in week 4. | 06:39 |
| thumper | rockstar: have a good break | 06:40 |
| === 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 | ||
| bac | hello adeuring | 14:37 |
| 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:38 |
| 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:53 |
| adeuring | OK, I'll take https://code.edge.launchpad.net/~sinzui/launchpad/front-page-footer-0/+merge/27311 | 14:54 |
| bac | ok | 14:54 |
| === 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 | ||
| 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:12 |
| 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:13 |
| sinzui | bac: I think I demonstrated that in the test. | 15:15 |
| * sinzui looks | 15:16 | |
| sinzui | bac, waaa.. | 15:17 |
| 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:18 |
| sinzui | :( | 15:19 |
| 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:20 |
| sinzui | no way, | 15:21 |
| sinzui | bac, http://pastebin.ubuntu.com/448268/ | 15:21 |
| 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:22 |
| 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:23 |
| sinzui | It was just after midnight I screwed up. | 15:24 |
| sinzui | bac: http://pastebin.ubuntu.com/448271/ | 15:24 |
| bac | sinzui: see, curtis, turning into a pumpkin is just a metaphor. you really should stop work after midnight. | 15:26 |
| adeuring | sinzui: r=me; two nitpicks | 15:47 |
| === 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 | ||
| sinzui | adeuring, thanks | 15:47 |
| bac | sinzui: r=bac, with a couple of changes and one absurd request | 16:01 |
| === 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 | ||
| sinzui | thank bac | 16:02 |
| sinzui | bac ping | 16:31 |
| 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:32 |
| bac | wellispecificallyhatedthelackofseparatorsinthename | 16:33 |
| sinzui | We can start by making good api name. maybe release_finder_url_pattern? | 16:33 |
| bac | sinzui: hey that's much better! | 16:37 |
| bac | cause, you know what? it *is* an URL pattern | 16:37 |
| 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:38 |
| 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:39 |
| 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 | 16:40 |
| === deryck_ is now known as deryck | ||
| === deryck is now known as deryck[lunch] | ||
| 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:13 |
| adeuring | gary_poster: I'll look | 18:14 |
| 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:15 |
| gary_poster | thank you adeuring | 18:16 |
| 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:20 |
| 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:21 |
| === 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 | ||
| bac | sinzui: you are in the topic queue but i think that is a mistake, right? i see nothing on activereviews | 18:35 |
| sinzui | bac: yes I think I am done | 18:36 |
| === 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 | ||
| 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 | 19:57 |
| === 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 | ||
| 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:02 |
| 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:04 |
| 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:05 |
| bac | EdwinGrubbs: ok. that makes sense. the branch proper looks fine...i was just confused. | 20:10 |
| bac | EdwinGrubbs: finally, i assume there was no real benefit to returning a generator instead of the storm resultset as you do now. | 20:11 |
| bac | oh, i see they were filtering the results too, which you don't have to do now | 20:12 |
| EdwinGrubbs | bac: right, the generator is just overhead | 20:13 |
| bac | sinzui: is your other branch going to make it today? | 21:48 |
| sinzui | No | 21:48 |
| 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:49 |
| 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:55 |
| 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:56 |
| sinzui | c.l.__init__ has a lot in it still because removing all causes cyclic import problems | 21:57 |
| 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:58 |
| 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 | 21:59 |
| sinzui | You have a good point. | 22:00 |
| sinzui | gary talked me off the ledge this week so the scope is safe | 22:00 |
| === matsubara is now known as matsubara-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!