| jpds | EdwinGrubbs: I have https://code.launchpad.net/~jpds/launchpad/fix-distromirrorstatus-colours/+merge/24378 if you're still around. | 01:09 |
|---|---|---|
| EdwinGrubbs | jpds: what's a url where I can see the color? | 01:12 |
| jpds | EdwinGrubbs: Compare: https://edge.launchpad.net/ubuntu/+archivemirrors and https://launchpad.net/ubuntu/+archivemirrors | 01:12 |
| EdwinGrubbs | jpds: r=me | 01:15 |
| === EdwinGrubbs 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 | ||
| jpds | EdwinGrubbs: Cool, can you please send it to PQM? :) | 01:15 |
| EdwinGrubbs | jpds: sure | 01:16 |
| thumper | hello friendly reviewer: https://code.edge.launchpad.net/~thumper/launchpad/code-email-permissions/+merge/24390 | 03:50 |
| thumper | mwhudson: ^^ it is very small | 03:53 |
| mwhudson | thumper: looking | 03:53 |
| mwhudson | thumper: done | 03:55 |
| thumper | ta | 03:55 |
| thumper | mwhudson: how about: | 04:29 |
| thumper | Permission to mark a merge proposal as approved checks launchpad.Edit | 04:29 |
| thumper | of the target branch, or membership of the review team on the target | 04:29 |
| thumper | branch. For import branches launchpad.Edit also checks the registrant | 04:29 |
| thumper | of the code import if there is one, and membership of vcs-imports. So | 04:29 |
| thumper | if someone is attempting to review something on an import branch, but | 04:29 |
| thumper | they don't have launchpad.Edit but are a member of the review team, | 04:29 |
| thumper | then a check against the code import is done. | 04:30 |
| mwhudson | thumper: it almost feels like it needs a diagram :-) | 04:30 |
| mwhudson | thumper: but that'll do | 04:31 |
| thumper | it probably does | 04:31 |
| === noodles775 changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [noodles*3] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| noodles775 | jelmer: when you've time, I've got 3 boring refacting branches that could benefit from your eyes :) | 09:03 |
| jelmer | noodles775: sure, np :-) | 09:09 |
| === jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [noodles*3] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| noodles775 | jtv: so regarding makeJob(), I'm not sure what the issue is? (Given that you can provide your own implementation on TranslationTemplatesBuildJob) | 10:04 |
| noodles775 | Or you mean the issue is that I haven't done that as part of the branch. | 10:05 |
| === jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| jtv | noodles775: no, just wanted to let you know that the direction could lead you to clashes with the existing code, is all | 10:06 |
| jtv | noodles775: I hadn't realized that it was the intention to allow it to be a no-op | 10:07 |
| noodles775 | It wasn't the intention to allow it to be a no-op, but derived classes *must* implement it either way. | 10:08 |
| noodles775 | jtv: all the other derived classes are *Build objects, and so SPRecipeBuild.makeJob() returns an SPRBuildJob. | 10:09 |
| noodles775 | In your case, you've got a BuildJob object implementing makeJob() (which is unexpected from my end), but simply returning self probably makes the most sense? | 10:10 |
| jtv | noodles775: I must say I'm still getting my head around the boundary between builds and buildfarmjobs in your branch | 10:10 |
| noodles775 | jtv: did you look at http://people.ubuntu.com/~wgrant/launchpad/buildfarm/new-build-model-again.png | 10:11 |
| jtv | noodles775: only an older version I think | 10:12 |
| noodles775 | Basically, BuildFarmJob is just the base information common to all things that run on the buildfarm. But yeah, it is confusing. It'll be helped later when we get to clean up the queue side of things (bug 570939) | 10:13 |
| mup | Bug #570939: Replace BPJ and SPRBJ with IBuildFarmJob.job <Soyuz:Triaged> <https://launchpad.net/bugs/570939> | 10:13 |
| noodles775 | jelmer: Just to be sure, you're looking at this one right? https://code.edge.launchpad.net/~michael.nelson/launchpad/567922-binarypackagebuild-packagebuild-3/+merge/24327 | 10:15 |
| noodles775 | jtv: let me know if you've got other questions or things I can fix up in that branch. Thanks! | 10:17 |
| jelmer | noodles775: no, I was looking at -2 | 10:17 |
| jtv | noodles775: got some waiting on my other machine, hang on | 10:17 |
| noodles775 | jelmer: That's the one that jtv has already started. Sorry for the confusion... there are still 3 branches left, and the first one is -3. | 10:17 |
| jelmer | noodles775: ah, ok - I'll have a look at that one then | 10:18 |
| jelmer | noodles775: I noticed that your branches touches on some code that I've recently changed | 10:18 |
| jelmer | noodles775: I've reordered some methods in buildbase.py | 10:18 |
| noodles775 | jelmer: yeah, I expect to be resolving conflicts for the next while :) | 10:19 |
| jtv | noodles775: sorry for delays... some nasty distractions | 10:26 |
| noodles775 | jtv: no problem (just glad you're still there :)). | 10:26 |
| jtv | noodles775: l.61 of the diff, lib/lp/buildmaster/interfaces/packagebuild.py: getUploaderCommand... I know this is not your fault but what does "the command to run as the uploader" mean? That the class is all about running one particular command, and it will be run under the uploader's identity? | 10:27 |
| noodles775 | Yes, personally i think it might be better named getUploadCommand(). | 10:29 |
| jelmer | jtv: fwiw I'm landing a branch that removes it (we're importing the uploadprocessor directly now rather than through subprocess) | 10:30 |
| noodles775 | Even better :) | 10:30 |
| jtv | yep! | 10:30 |
| jtv | Same for getUploadLeaf? | 10:31 |
| jtv | noodles775: also, note that the slave build id has been replaced with the slave build cookie, which has (I hope) no discernible internal structure | 10:31 |
| jelmer | jtv: no, just getUploaderCommand and getUploadLogContent | 10:32 |
| noodles775 | jtv: also, you're right about the makeJob - in that the docstring implies that its sole purpose is to create the services job, rather than return the 'specific_job' which links the services job. I'll update it and add a :return:. | 10:32 |
| jtv | noodles775: thanks... yes, and since the implementation wasn't in the diff... | 10:33 |
| jtv | jelmer: shame, because its interface entry could do with a healthy dose of violent renewal as well | 10:33 |
| jelmer | jtv: agreed | 10:33 |
| jtv | noodles775: the docstring for getUploadLeaf I find confusing too—starting with the "Upload" part of the name. "Build things" will be stored there... for the purpose of upload? | 10:34 |
| jtv | Discuss. | 10:34 |
| noodles775 | jtv: yeah, as noted, I didn't even read the docstrings - I was just replicating the interface. But getUploadDirName (cf. with getUploadDir)? | 10:38 |
| noodles775 | "Return the name of the directory where the uploaded files will be stored."? | 10:39 |
| jtv | noodles775: step in the right direction, though I do think that Leaf is good for explaining the assumed directory structure. | 10:39 |
| jtv | noodles775: uploaded files, or files to be uploaded? I'm not familiar with this stuff to be sure. | 10:39 |
| noodles775 | Right, good point. I assume it's the files to be uploaded. | 10:41 |
| jtv | noodles775: might as well get some clarity into these murky depths. :) | 10:42 |
| jtv | noodles775: ready for more? | 10:45 |
| noodles775 | jtv: yep... I'm just renaming/updating the docstrings, but can do that while we chat :) | 10:47 |
| jtv | noodles775: ok... did you get the note about slave build ids not existing any more? | 10:47 |
| jtv | One of the docstrings describes their usual structure, but that no longer applies | 10:48 |
| jtv | noodles775: (also, that diagram you sent me still has arrows pointing the wrong way afaics) | 10:49 |
| noodles775 | BTW: just checked, it is actually the location where the uploaded files will be stored (ie. /incoming/...) | 10:49 |
| noodles775 | jtv: I did, but I wasn't updating the interface... but will, did you decide on a var name? buildd_cookie? | 10:50 |
| jtv | noodles775: I'd say just replace "id" with "cookie." | 10:51 |
| jtv | It's basically like free cookies. | 10:51 |
| noodles775 | Done. | 10:51 |
| jtv | noodles775: sorry, more urgent stuff elsewhere. :) Where was I? Oh yes. In the getUploadLeaf docstring, there's a :param now: that says "now" is the time to use. But for what? | 11:10 |
| jtv | noodles785: In the getUploadLeaf docstring, there's a :param now: that says "now" is the time to use. But for what? | 11:15 |
| noodles785 | jtv: "The datetime to use when constructing the leaf directory name."? | 11:16 |
| === noodles785 is now known as noodles775 | ||
| jtv | noodles775: assuming that's actually true, perfect, thanks. (And that goes to show that this has actual information value :-) | 11:17 |
| noodles775 | jtv: should I push these changes: http://pastebin.ubuntu.com/424513/ | 11:17 |
| * jtv looks | 11:17 | |
| jtv | noodles775: is there a more formal way of saying "specific job" in the makeJob docstring? | 11:18 |
| noodles775 | Hmm... "The object linking the services job with this build farm job."? | 11:18 |
| * noodles775 looks forward to 'specific_job' dying a slow death. | 11:19 | |
| jtv | :) | 11:19 |
| jtv | noodles775: but that's saying "object," which makes it seem even vaguer somehow... | 11:20 |
| jtv | (Also, line 102 of that pastebin still mentions "build id." Apart from that, it looks good to me) | 11:20 |
| noodles775 | jtv: the XXX explains the awkward situation with the specific job (you can only see the first line in the diff). | 11:21 |
| jtv | noodles775: ok | 11:22 |
| jtv | noodles775: then those changes are good afaic, and I've only got two more notes about the branch as a whole. | 11:22 |
| noodles775 | OK, those changes pushed. Shoot. | 11:22 |
| jtv | You're creating class hierarchies with TestCase-derived base classes as helpers. | 11:23 |
| jtv | It'd be better not to derive those from TestCase, but use them as mixins instead. | 11:23 |
| * noodles775 looks | 11:24 | |
| jtv | Where they have setUp methods, you'll want to rename those and call them explicitly from the subclass setUp methods so as not to confuse super(), but it saves you awkwardness like deriving classes from TestCase twice (once through your own base class, once through TestClassWithFactory) | 11:24 |
| jtv | It also avoids the impression that those base classes are themselves test cases. | 11:24 |
| noodles775 | Right... doing so now. | 11:26 |
| jtv | It's in a bunch of places. | 11:26 |
| jtv | noodles775: client crash | 11:27 |
| jtv | noodles775: turns out the test-class inheritance was my last note (the other one I had has just evaporated), so once that's cleared up, you're good to land. | 11:40 |
| noodles775 | jtv: unfortunately, landing will be a while off yet (it's one of currently 7 branches in a pipe, dependent on a db-schema change that can't land until it's all finished). But approval is a step forward :) | 11:42 |
| jtv | noodles775: whoops, I typed that on autopilot. | 11:44 |
| jelmer | how is flush_database_updates() different from the other flush/commit methods? | 11:49 |
| noodles775 | jtv: http://pastebin.ubuntu.com/424531/ | 11:49 |
| noodles775 | jelmer: it's faster than committing (just going on what I've heard), but it could be that I should be using flush instead of flush_database_updates... I can try using store.flush() instead. | 11:50 |
| noodles775 | jelmer: store.flush() works, and since it is only needed in one test, I moved it there instead of in the setUp: http://pastebin.ubuntu.com/424538/ | 12:03 |
| jelmer | ah, great | 12:03 |
| * noodles775 gets rid of the other instance of flush_database_updates in the same file. | 12:04 | |
| mrevell | Hi jelmer, may I add a text change branch to the review queue please? | 12:42 |
| === mrevell changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles*2, mrevell] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| mrevell | jelmer, I've assumed "yes" :) The branch is here: https://code.edge.launchpad.net/~matthew.revell/launchpad/whats-new-10-04/+merge/24408 | 12:43 |
| mrevell | I'm off for lunch for a bit | 12:43 |
| === mrevell is now known as mrevell-lunch | ||
| jelmer | mrevell-lunch: sorry, I was away for lunch | 13:11 |
| jelmer | mrevell-lunch: yeah, adding something to the queue isn't a problem | 13:11 |
| henninge | adiroiban: did you see the test failure? | 13:26 |
| adiroiban | henninge: :) yes. I fixed the one from the translations, but I have no idea why the registry windmill tests have failed | 13:27 |
| adiroiban | some for the other translation windmill tests | 13:28 |
| adiroiban | but I'm working on solving them | 13:30 |
| adiroiban | just that I can not reproduce them on my computer | 13:33 |
| danilos | adiroiban, hi, any reason you haven't gotten https://code.edge.launchpad.net/~adiroiban/launchpad/bug-402235/+merge/23973 landed? | 13:42 |
| danilos | adiroiban, or https://bugs.edge.launchpad.net/rosetta/+bug/487137 for that matter? | 13:43 |
| mup | Bug #487137: Allow Rosetta admins to create custom language codes <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/487137> | 13:43 |
| adiroiban | danilos: yes. the permission problem is haunting me again and I don't know how should I map 3 roles to 2 zope permissions ... | 13:45 |
| adiroiban | I know this is not possible | 13:45 |
| adiroiban | and one suggestion was to create an adapter and delegate the custom language code part of IProduct | 13:45 |
| adiroiban | but thinks are not that simple | 13:46 |
| adiroiban | a simple solution would be to add launchpad.CustomLanguageCodeAdmin permission | 13:46 |
| danilos | adiroiban, ok, just wondering because today is the last day to land stuff for 10.04, but I won't be able to help much with these things | 13:46 |
| danilos | adiroiban, (i.e. we are very busy with many things today trying to get generation of templates in proper shape) | 13:47 |
| adiroiban | danilos: don't worrr | 13:47 |
| adiroiban | danilos: don't worry | 13:47 |
| adiroiban | I can imagine that people are busy now | 13:47 |
| === noodles775 changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| adiroiban | also for that bug, I think I will have to talk with sizui and discuss what is the best option | 13:49 |
| adiroiban | in the same time, I was also busy with other problems. | 13:49 |
| adiroiban | btw. sorry for leaving yesterday without a notice :) | 13:50 |
| danilos | adiroiban, anyway, I think using TranslationsAdmin should be fine, but I might be missing something :) | 13:50 |
| danilos | adiroiban, also, I've sent you a comment on bug 548375 if you haven't seen it | 13:50 |
| mup | Bug #548375: Rosetta import empty strings (e.g. "") from .po files <rosetta-imports> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/548375> | 13:50 |
| danilos | adiroiban, heh, no worries :) | 13:51 |
| adiroiban | danilos: translationAdmin is not ok, since product owners are also TranslationAdmin | 13:53 |
| adiroiban | and the should not be allowed to change custom language codes (as far as I understoond from the bug report) | 13:53 |
| danilos | adiroiban, right, it's probably something for us to reconsider :) | 13:54 |
| === mrevell-lunch is now known as mrevell | ||
| adiroiban | allowing product owners to change custom language codes will make things ... plain :) | 13:55 |
| adiroiban | henninge: can you please try to resubmit that branch again? I think those windmill failers were transient as I can no longer reproduce them | 14:04 |
| adiroiban | danilos: You still don't have the Belgium visa? | 14:05 |
| danilos | adiroiban, I don't need it anymore since Dec 2009 | 14:05 |
| adiroiban | ah. nice. and will you be at UDS? | 14:06 |
| danilos | adiroiban, no, sorry | 14:07 |
| henninge | adiroiban: I can do that, sure. | 14:10 |
| adiroiban | henninge: do you think you can also push my other branches that are approved (ie. https://code.edge.launchpad.net/~adiroiban/launchpad/bug-146178/+merge/24265 ) or should I talk with bac ? | 14:11 |
| henninge | adiroiban: if they are approved, I can land them. | 14:11 |
| bac | adiroiban: i attempted to land your branch last night but, again, ec2 ate it | 14:11 |
| bac | adiroiban, henninge: i will attempt again shortly. | 14:12 |
| henninge | bac: ok, cool | 14:12 |
| adiroiban | adeuring: is there anything I need to do for this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-402235/+merge/23973 ? | 14:12 |
| adeuring | adiroiban: I think I found a few spelling mistakes ;) Once you fix them, I can run the ec2 | 14:13 |
| adiroiban | adeuring: they should be fixed. I have also merged with devel and fixed the conflicts | 14:14 |
| adeuring | adiroiban: ah, sorry I did not notice this. I'll start the ec test right now | 14:15 |
| adiroiban | but I think it is my fault for only pushing those changes without adding a new comment to the MP | 14:15 |
| adeuring | adiroiban: would have been better to ping me about the pushes ;) | 14:15 |
| adeuring | adiroiban: and sorry, I should have recommended earier to ping me... | 14:16 |
| === StevenK changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles*2,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === bigjools changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles*2,StevenK, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| bigjools | soyuz review overload | 14:50 |
| === jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles,StevenK,bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| * noodles775 takes a look at the other branches | 14:54 | |
| leonardr | gary, if you have time i'd like you to look into the mystery of https://code.edge.launchpad.net/~leonardr/lazr.restful/fix-remove-recipe/+merge/24423 | 14:56 |
| leonardr | why do you think the lazr.restful tests pass but the lazr.restfulclient tests fail? | 14:57 |
| === noodles775 changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles,bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| gary_poster | leonardr: I'll take a glance, though I'm kind of swamped today. If I have any immediate ideas I'll share them though. I'll be able to take a look within the next hour (which is also when we are scheduled to have our call). | 14:58 |
| leonardr | gary: the reason i'm picking on you is it's a very zope-security problem. if you can think of someone else who might know, i'll ask them | 14:58 |
| gary_poster | I'm your best bet, I think. You know who the other suspects are. | 14:58 |
| leonardr | jelmer, i'd like to add https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/delete-entry/+merge/24421 to the queue | 15:00 |
| leonardr | bac, want to solve a zope mystery? | 15:00 |
| jelmer | leonardr: sure, np | 15:01 |
| bac | leonardr: ? | 15:01 |
| === leonardr changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles,bigjools,leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| noodles775 | StevenK: did you check that the comment about binary expiry also applies for sources (ie. that just that fact that expires is set is enough to deny the copy)? | 15:01 |
| leonardr | bac: https://code.edge.launchpad.net/~leonardr/lazr.restful/fix-remove-recipe/+merge/24423 | 15:01 |
| jelmer | noodles775: what's the policy wrt updating import ordering in existing files? | 15:01 |
| leonardr | the zope security proxy is protecting objects when i run lazr.restfulclient tests, but not when i run lazr.restful tests on the same objects | 15:01 |
| bac | leonardr: interesting. unfortunately i don't have time ATM to look at it. | 15:02 |
| noodles775 | jelmer: Generally if it's not going to blow the diff out, usually we request people to order them as a drive-by, but it's not policy afaik. | 15:02 |
| leonardr | bac: ok, np | 15:02 |
| jelmer | noodles775: ok, thanks | 15:02 |
| StevenK | noodles775: I was playing since-wgrant-says card, but I'm having a dig now | 15:04 |
| wgrant | I wondered that myself a few weeks ago, but played the soyuz-code-says card and decided not to think about it any more. | 15:05 |
| wgrant | The mechanics of LFA expiry still confuse me slightly. | 15:05 |
| gary_poster | leonardr: almost certainly the reason for a problem like that is that, in one case, the source of the objects is security wrapped, and in the other, it is not. Since it is "viral," a simple change at the beginning of the process will result in this. | 15:07 |
| gary_poster | leonardr: I will try to look at details with you a little later this hour | 15:07 |
| leonardr | gary: great | 15:07 |
| StevenK | noodles775: Right, so death row processing will certainly expire out sources | 15:10 |
| noodles775 | StevenK: r=me... I added a small comment on the MP (just about updating the comment and perhaps the test to demo this). | 15:11 |
| StevenK | noodles775: Source and binary expiration is slightly seperate. Just because a binary is expired doesn't mean a source is | 15:13 |
| StevenK | noodles775: The comment in the code, or the test? | 15:14 |
| noodles775 | StevenK: Yep, I just meant that it should be obvious when looking at the source check *why* we are only checking that expires is set, rather than that it's in the past (ie. "See above comment regarding expires."). | 15:16 |
| StevenK | noodles775: TBH, I thought that was nicely covered already by the binary comment. | 15:17 |
| noodles775 | StevenK: and I think the MP has more details about the test change I was suggesting (just not using a 1970 date, so that the test also shows that the expiry just has to be set to raise the exception). | 15:17 |
| noodles775 | StevenK: yes, it is, if it's right above it then fine (I didn't notice in the diff, just looked locally at trunk). | 15:18 |
| StevenK | noodles775: Yeah, the comments are about 6 lines apart | 15:18 |
| noodles775 | Perfect, forget that then :) | 15:18 |
| StevenK | And I was using 1970 since the test for expired binaries did the same | 15:19 |
| noodles775 | StevenK: yep, but looking at the test on its own, you'd expect it to fail for an expires in the future (well, I would). But up to you, I'm not fussed about it. | 15:20 |
| === noodles775 changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles,leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| * StevenK gives ec2 another chance | 15:21 | |
| wgrant | Can someone please ec2 lp:~wgrant/launchpad/delete-more-stuff? Two attempts to land it vanished yesterday. | 15:28 |
| wgrant | (my others landed after two or three attempts... yay reliability) | 15:28 |
| noodles775 | bigjools: given that you're change will also require updating lp-production-configs, should you be wrapping the actual config.buildmaster.bzr_builder_sources_list in a try/except? | 15:31 |
| noodles775 | wgrant: yep, trying now. | 15:31 |
| wgrant | noodles775: Thanks. | 15:32 |
| noodles775 | bigjools: also, why not catch KeyError rather than StandardError when substituting the series? | 15:32 |
| bigjools | noodles775: it returns None if it doesn't exist | 15:33 |
| bigjools | I think! | 15:33 |
| noodles775 | bigjools: I just tried locally - AttributeError: No section key named bzr_builder_sources_list. | 15:33 |
| bigjools | re. second point, I am shamefully copy & wasting the code in adapters/archivedependencies.py that does a similar thing | 15:33 |
| bigjools | noodles775: crap, I'll fix that, thanks for noticing | 15:34 |
| noodles775 | np. Great. | 15:34 |
| noodles775 | bigjools: you could rework the second test (or add a separate test) so it shows the failure when the config option is not set? | 15:39 |
| bigjools | noodles775: yep, I can add another test | 15:39 |
| bigjools | noodles775: KeyError doesn't catch the failure | 15:50 |
| noodles775 | bigjools: really? I was just doing "hey %(expected_key)s" % {'actual_key': 'blah'}, but maybe I obviously missed something? | 15:55 |
| bigjools | noodles775: I think there's a sufficiently broad range of possible errors that it makes sense to catch a base exception doesn't it? | 15:55 |
| noodles775 | bigjools: yep. | 15:55 |
| bigjools | at least that's why it's in the other code IIRC | 15:56 |
| noodles775 | Especially if you include getting the config option in the same try/except, or are you adding a separate one? | 15:56 |
| bigjools | it's separate | 15:56 |
| bigjools | noodles775: http://pastebin.ubuntu.com/424644/ | 15:57 |
| noodles775 | bigjools: isn't it an error if the config option is not set? (ie. do you want to log.error?) | 15:58 |
| bigjools | noodles775: no, it's fine, since it will be unset in production for a while | 15:59 |
| noodles775 | bigjools: ok, great. | 15:59 |
| === matsubara is now known as matsubara-lunch | ||
| === gary_poster is now known as gary-lunch | ||
| === matsubara-lunch is now known as matsubara | ||
| === salgado is now known as salgado-lunch | ||
| henninge | danilos: where's the branch/mp | 17:14 |
| henninge | ? | 17:14 |
| danilos | henninge, email's coming up | 17:14 |
| danilos | henninge, I've CCed you directly | 17:14 |
| henninge | danilos: got it | 17:15 |
| danilos | henninge, cool! | 17:15 |
| === rockstar changed the topic of #launchpad-reviews to: On call: jelmer, rockstar || reviewing: noodles || queue: [noodles,leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| henninge | danilos: a lot of copy-and-paste in the test ;-) | 17:26 |
| henninge | danilos: The class docstring, the class attributes | 17:26 |
| henninge | danilos: and the comment on each test method is identical ... | 17:26 |
| danilos | henninge, heh, I don't know about those things :) | 17:27 |
| henninge | danilos: Can you give that some love, please? | 17:27 |
| danilos | henninge, sure | 17:27 |
| henninge | danilos: don't feign ignorance, won't work on me! :-P | 17:27 |
| bac | adiroiban: i resent your branch to ec2 a good while ago. let's hope this one works | 17:28 |
| henninge | danilos: the other thing I wonder about is the indention of the "warning". The previous code did it write, your version is not in harmony with our style. ;) | 17:29 |
| henninge | (i.e. it's wrong ;) | 17:29 |
| henninge | s/write/right/ | 17:29 |
| danilos | henninge, lint was complaining about it | 17:29 |
| henninge | what did it complain about? | 17:29 |
| henninge | danilos: anyway, it's not a strong point. Basically this branch is good. | 17:30 |
| henninge | r=me | 17:30 |
| danilos | henninge, you are going to like this: "519: [W6501, TranslationImportQueueEntry.getGuessedPOFile] Specify string format arguments as logging function parameters" | 17:31 |
| henninge | ;-) | 17:31 |
| henninge | danilos: but I was just talking about the multiline-handling. | 17:31 |
| danilos | henninge, I know what are you talking about, but why do you say it's not according to style guide? | 17:32 |
| henninge | https://dev.launchpad.net/PythonStyleGuide#Multiline%20braces | 17:33 |
| henninge | danilos: I'd think it should be like this: | 17:35 |
| henninge | warning = ( | 17:35 |
| henninge | "%s: can't approve entry %d ('%s') " | 17:35 |
| henninge | "because entry %d is in the way." % ( | 17:35 |
| henninge | potemplate.title, self.id, self.path, | 17:35 |
| henninge | existing_entry.id)) | 17:35 |
| henninge | but as I said, not a strong point for me ... | 17:35 |
| danilos | henninge, well, not really | 17:35 |
| danilos | henninge, according to that, only stuff after % should be reformatted for each bit to be on a separate line | 17:35 |
| danilos | henninge, the initial brace is an alignment parenthesis, it's not a multi-element one :) | 17:36 |
| henninge | danilos: right, I am just trying to apply the style. | 17:36 |
| danilos | henninge, only stuff after % (string formatting) is a tuple | 17:36 |
| henninge | true | 17:36 |
| danilos | henninge, well, I am applying the multiline function definitions style myself so I think it's good :P | 17:37 |
| * danilos goes check multiline fundefs style now ;) | 17:37 | |
| henninge | danilos: they allow both, I jut wrote them the other week ... ;) | 17:37 |
| danilos | henninge, I know :) | 17:37 |
| henninge | anyway, do it in whatever way you like. I have to go now ... ;-) | 17:38 |
| henninge | Launchpad is sooooo slooooow today ... | 17:40 |
| === gary-lunch is now known as gary_poster | ||
| === salgado-lunch is now known as salgado | ||
| === rockstar changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === gary_poster_ is now known as gary_poster | ||
| === matsubara is now known as matsubara-afk | ||
| === salgado is now known as salgado-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!
