| === StevenK changed the topic of #launchpad-reviews to: On call: StevenK || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === Edwin is now known as Guest62855 | ||
| === jtv changed the topic of #launchpad-reviews to: On call: StevenK, jtv || reviewing: -,- || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === jtv is now known as jtv_ | ||
| * mwhudson waves https://code.edge.launchpad.net/~mwhudson/launchpad/move-SpecificationDepCandidatesVocabulary/+merge/33611 around if anyone is free | 06:55 | |
| mwhudson | it's only waffer thin | 06:56 |
|---|---|---|
| StevenK | mwhudson: 613 lines is waffer thin? :-) | 07:04 |
| mwhudson | StevenK: it's mostly just moving code around | 07:04 |
| StevenK | mwhudson: It looks fine to me, but I'd also need thumper to look at it, and I think he's afk, so you can beg jtv_ if you'd like to land it soonish | 07:08 |
| jtv_ | Do I hear a grovel approaching? | 07:08 |
| mwhudson | jtv_: if you could take a look | 07:08 |
| === jtv_ is now known as jtv | ||
| mwhudson | i'm not in a massive hurry | 07:08 |
| jtv | mwhudson: btw if you want a review, do try naming the OCRs so they know there's customers! | 07:09 |
| jtv | mwhudson: I'll take it. | 07:09 |
| jtv | StevenK: I worked through the overnight backlog. | 07:09 |
| jtv | Forgot to update the topic though. | 07:09 |
| jtv | On call: StevenK, jtv || reviewing: -,mwhudson || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | 07:10 |
| jtv | I mean… | 07:11 |
| === jtv changed the topic of #launchpad-reviews to: On call: StevenK, jtv || reviewing: -,mwhudson || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| mwhudson | jtv: i never know who's on call, even if it says so in the /topic :-) | 07:12 |
| mwhudson | jtv: thanks a lot | 07:12 |
| jtv | mwhudson: why is the copyright on lib/lp/blueprints/vocabularies/configure.zcml dated 2009? | 07:26 |
| mwhudson | jtv: you get one guess | 07:28 |
| jtv | "I copied it from a file that said 2009"? | 07:28 |
| mwhudson | jtv: bingo | 07:28 |
| mwhudson | fixed | 07:28 |
| jtv | What do I win? | 07:29 |
| mwhudson | jtv: are you going to UDS-N? | 07:29 |
| jtv | No | 07:29 |
| mwhudson | mm | 07:29 |
| mwhudson | will 'a beverage next time we meet' do? | 07:29 |
| jtv | absolutely. | 07:29 |
| mwhudson | deal | 07:29 |
| jtv | *grin* | 07:29 |
| mwhudson | jtv: are you going to be done soon? | 07:31 |
| jtv | mwhudson: _filter_specs could do with a brief docstring. | 07:31 |
| jtv | mwhudson: I think so, yes | 07:31 |
| mwhudson | don't want to rush you, just want to know if to hang on here for another few minutes | 07:31 |
| jtv | mwhudson: won't be long. | 07:32 |
| mwhudson | ok | 07:32 |
| mwhudson | jtv: yes, i wonder what that function does... | 07:32 |
| jtv | In _doSearch, you may want to use "%(query)s, %(query)s, %(query)s" % {'query': quoted_query} | 07:33 |
| mwhudson | jtv: http://pastebin.ubuntu.com/483286/ | 07:34 |
| jtv | mwhudson: Just a formality. Its name also doesn't follow the guidelines, and the code is mis-formatted… I'm not planning to be too difficult about code you only moved though. | 07:34 |
| mwhudson | jtv: i don't want to touch the code | 07:34 |
| mwhudson | it will probably all get thrown away in the next branch anyway | 07:35 |
| jtv | mwhudson: that's actually helpful, thanks. | 07:35 |
| mwhudson | jtv: sorry for not saying so in the merge proposal then | 07:36 |
| jtv | mwhudson: what I don't get is… I _think_ this is code you're moving, but I only see it added. Aren't you also removing it somewhere else? | 07:39 |
| mwhudson | jtv: haha, it's _supposed_ to be removed from dbobjects | 07:41 |
| mwhudson | jtv: but clearly that was a different branch :( | 07:42 |
| mwhudson | jtv: need to run for a bus, can you comment on the mp and i'll sort it tomorrow | 07:42 |
| jtv | OK | 07:42 |
| mwhudson | actually... | 07:43 |
| mwhudson | pushing a fix | 07:43 |
| jtv | get that bus! | 07:43 |
| mwhudson | but now running | 07:43 |
| === StevenK changed the topic of #launchpad-reviews to: On call: jtv || reviewing: mwhudson || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | StevenK: bowing out? Good night then! | 08:09 |
| StevenK | jtv: From OCR at least :-) | 08:10 |
| jtv | ah ok | 08:10 |
| StevenK | jtv: Going to be in and out until the call, so no point people pinging me if I'm afk | 08:10 |
| === Guest17053 is now known as jelmer | ||
| === 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 | ||
| jtv | lifeless: or maybe this is something you can approve for CP? https://code.launchpad.net/~jtv/launchpad/bug-623865/+merge/33617 | 10:49 |
| jtv | (it can wait for danilo to come back, but I'm eager to shave off a few more seconds :) | 10:49 |
| === jelmer is now known as Guest61071 | ||
| === Guest61071 is now known as jelmer | ||
| === jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === danilo_ is now known as danilos | ||
| === danilos is now known as danilo | ||
| === danilo is now known as danilos | ||
| === mrevell is now known as mrevell-lunch | ||
| === matsubara-afk is now known as matsubara | ||
| jml | bigjools, reviewing your big branch... | 13:23 |
| === mrevell-lunch is now known as mrevell | ||
| bac | hi jelmer, may i get a review? | 13:43 |
| jelmer | bac: Hi! | 13:43 |
| jelmer | bac: Yes, of course. | 13:43 |
| bac | jelmer: thanks. https://code.edge.launchpad.net/~bac/launchpad/bug-618148/+merge/33600 | 13:44 |
| === bac changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bigjools | jml: it's WIP | 13:50 |
| bigjools | so don't | 13:50 |
| jml | bigjools, too late. | 13:51 |
| jml | bigjools, it sucks a little that I couldn't have found that out over email. | 13:52 |
| bigjools | jml: the status is WIP! | 13:52 |
| jml | bigjools, not mentioned in the email | 13:52 |
| bigjools | assuming you're looking at my soyuz-enums branch? | 13:52 |
| jml | bigjools, yes, I was. | 13:52 |
| bigjools | I created it with the "ready for review" unchecked | 13:52 |
| bigjools | so if it's sending email, that's a bug | 13:52 |
| jml | bigjools, surely. | 13:53 |
| bigjools | in any case I was going to merge it rs=whoevcer | 13:53 |
| bigjools | since it's a purely mechanical change | 13:53 |
| bigjools | I'm sorry that you started reviewing it :( | 13:54 |
| jml | fair enough. | 13:54 |
| jml | that's ok | 13:54 |
| jml | I finished reviewing it too. | 13:54 |
| bigjools | !!!!!! | 13:54 |
| bigjools | it's 5k lines! | 13:54 |
| jml | 22 minutes ago. | 13:54 |
| bigjools | it's not finished yet either :) | 13:54 |
| jml | bigjools, yes, but it doesn't take very long to look at a changed import and decide that it's fundamentally sound :) | 13:55 |
| bigjools | :) | 13:55 |
| bigjools | jml: you can help me though if you have 5 mins | 13:55 |
| jml | bigjools, I do. | 13:55 |
| jml | I also have half a beef sandwich | 13:55 |
| bigjools | jml: I've got one failure left | 13:57 |
| bigjools | lp.services.scripts.tests.test_all_scripts.ScriptsTestCase.test_scripts | 13:57 |
| * bigjools pastes | 13:57 | |
| bigjools | jml: http://pastebin.ubuntu.com/483416/ | 13:58 |
| jml | I hate Python. :( | 13:58 |
| bigjools | there's 2 failures, but surprisingly to me the import at the bottom is one I added to get around circular imports elsewhere :/ | 13:58 |
| bigjools | In Soviet Russia, Python hates you. | 13:59 |
| jml | bigjools, perhaps that workaround is hurting now? | 13:59 |
| jml | bigjools, Python hates me here in big society Britain. | 13:59 |
| bigjools | jml: I dunno what else to do. It doesn't have any hearing on the other error though. | 13:59 |
| jml | bigjools, is ALLOW_RELEASE_BUILDS in the wrong __all__, perhaps. | 14:00 |
| bigjools | I'd love it if we actually had a big society :) | 14:00 |
| jml | bigjools, Python would still hate me. | 14:00 |
| bigjools | jml: ALLOW_RELEASE_BUILDS is in the right place | 14:00 |
| * jml fetches the branch. | 14:01 | |
| jml | I always forget if you separate your names with - or . | 14:02 |
| allenap | Does anyone want to give me a trivial for http://paste.ubuntu.com/483422/? I'll land it as part of lp:~allenap/launchpad/cache-experiment-roll-out. | 14:06 |
| jml | oh, I see. | 14:06 |
| jml | bigjools, test_all_scripts assembles one big error. That's confusing. | 14:08 |
| deryck | allenap, I can. What does the extra +f line do? | 14:08 |
| jml | although arguably justifiable. | 14:08 |
| allenap | deryck: It includes the base-name of each file in the tags table. So if you search for a tag like "shipit.py", you'll get the file. Works nicely with Emacs, but I'm not sure about Vim. | 14:09 |
| bigjools | jml: doesn't help me much :( | 14:09 |
| allenap | deryck: The --extra line might be better in my personal .ctags. | 14:09 |
| bigjools | jml: I wonder how many of our scripts really need to be tested like this | 14:10 |
| deryck | allenap, doesn't sound contraversial, though. I've never used that, but we can drop it later if it doesn't play well with vim. | 14:10 |
| jml | bigjools, it's a sanity check, that's all. | 14:10 |
| deryck | allenap, r=me | 14:10 |
| jml | hmm. given that ./bin/py ./scripts/gina.py -h works, the failure seems bogus. let me try running the actual test locally. | 14:10 |
| allenap | deryck: Okay, thanks :) | 14:12 |
| bigjools | jml: seems unnecessarily slow | 14:12 |
| bigjools | we should do them all in test_scripts, or all in their own test cases | 14:13 |
| jml | bigjools, gina -h, or the test? | 14:13 |
| bigjools | jml: test_scripts I mean | 14:13 |
| bigjools | it's duplicating tests, and Popen on zopeless scripts is not exactly quick :/ | 14:13 |
| jml | bigjools, it's running every script, and each script is quite slow (just try running gina.py -h!) | 14:13 |
| bigjools | my point. | 14:13 |
| jml | bigjools, at the very least, the test could be rewritten to make one test case per script | 14:13 |
| jml | that would make it interact more nicely with the test runner. | 14:14 |
| === jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jml | this test ensaddens me. | 14:17 |
| jml | bigjools, there's precedent in lp.services.scripts.tests for marking a script as "known broken" because of circular imports. | 14:18 |
| bigjools | jml: where? | 14:19 |
| jml | bigjools, luckily, software is built on a common law system of jurisprudence, so I can recommend that we happily ignore it. | 14:19 |
| jml | bigjools, lib/lp/services/scripts/tests/__init__.py | 14:19 |
| bigjools | jml: the point is that this test is shit, it's failing unnecessarily. | 14:19 |
| jml | yes. | 14:19 |
| bigjools | the script works IRL | 14:19 |
| jml | I wonder what's causing it to fail | 14:20 |
| jml | I *think* the answer is that it's not using ./bin/py | 14:20 |
| bigjools | interesting | 14:20 |
| jml | "./bin/py cronscripts/ppa-generate-keys.py -h" *does* fail, however. | 14:20 |
| bigjools | sigh | 14:21 |
| jml | which reduces my confidence in the bin/py guess. | 14:21 |
| bigjools | it fails w/o bin/py too | 14:21 |
| bigjools | FPOS circular import shit | 14:21 |
| jml | yes. | 14:22 |
| jelmer | bac: Did you see StevenK's comment on your branch? | 14:22 |
| bac | jelmer: i did not. /me assumed no one had looked at it. | 14:23 |
| * bac looks | 14:23 | |
| bac | jelmer: so, never mind. | 14:23 |
| jml | bigjools, I'll keep trying to figure out why gina is failing. | 14:24 |
| bigjools | jml: ok thank you. I'll do the same import trick on the other script too | 14:24 |
| bigjools | after I fix bug 623859 anyway | 14:25 |
| _mup_ | Bug #623859: edge rollouts broke CSS on revno 11435 <canonical-losa-lp> <Soyuz:In Progress by julian-edwards> <https://launchpad.net/bugs/623859> | 14:25 |
| jml | that's the second edge rollout broken by CSS in two weeks. | 14:29 |
| jml | what's causing them? | 14:29 |
| StevenK | jml: Er, why is gina failing? | 14:36 |
| jml | StevenK, that's the question. | 14:37 |
| StevenK | jml: I have a branch ready to land that changes gina | 14:37 |
| jml | StevenK, it's to do with bigjools's soyuz-enums branch | 14:37 |
| StevenK | Ahh | 14:37 |
| bigjools | huh? | 14:37 |
| bigjools | jml: that bug is a missing future import for "with" | 14:38 |
| bigjools | but the "with" has been around since revno 7xxx | 14:38 |
| jml | bigjools, what? | 14:38 |
| jml | bigjools, oh sorry, too many conversations :) | 14:38 |
| bigjools | the bug above | 14:38 |
| bigjools | :) | 14:38 |
| jml | I quite like the new ec2 mail subjects, if I do say so myself. | 14:40 |
| * bigjools wouldn't know | 14:41 | |
| jml | bigjools, http://pastebin.ubuntu.com/483446/ makes test_all_scripts have one test per script | 14:41 |
| bigjools | jml: I'll try it out in a sec | 14:42 |
| bigjools | jml: jeebus, that test_scripts takes nearly 4 minutes to finish | 14:49 |
| bigjools | trying out your patch now | 14:49 |
| bac | hi henninge | 14:52 |
| bac | henninge: where does your super import tool live? i'd like to reference it in the wiki. | 14:52 |
| bigjools | jml: nice chance, it's good to see progress through the scripts. | 14:54 |
| bigjools | still doesn't help me :) | 14:54 |
| jml | bigjools, it helps me help you. | 14:54 |
| bigjools | heh | 14:54 |
| bigjools | jml: if I run bin/py gina/py it works, running it standalone doesn't | 14:56 |
| bigjools | gina.py even | 14:56 |
| bigjools | jml: argh I can see the problem | 14:58 |
| * bigjools fixes it | 14:58 | |
| bigjools | can someone approve this one-liner to restore edge rollouts please: https://code.edge.launchpad.net/~julian-edwards/launchpad/bug-623859/+merge/33640 | 15:03 |
| === jelmer_ is now known as Guest3182 | ||
| === Guest3182 is now known as jelmer | ||
| rockstar | bigjools, r=me | 15:07 |
| bigjools | thanks rockstar | 15:08 |
| === jelmer is now known as Guest48768 | ||
| === noodles775 changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: bac || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| noodles775 | Hi Guest48768, if you've time, the MP is: https://code.edge.launchpad.net/~michael.nelson/launchpad/remove-shortlist-getPublishedReleases/+merge/33645 | 15:25 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: bac || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| Guest48768 | noodles775: Yep, sure - will have a look. | 15:28 |
| === Guest48768 is now known as jelmer | ||
| === jelmer changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: noodles775, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === matsubara is now known as matsubara-lunch | ||
| jelmer | noodles775: r=me, thanks for the cleanups! | 16:11 |
| noodles775 | Thanks jelmer | 16:15 |
| sinzui | deryck, do you have time to look at the changes I made to script permissions in https://code.edge.launchpad.net/~sinzui/launchpad/dsp-bug-counts-1/+merge/33257 ? I think it needs your (or gmb's) judicious opinion to land | 16:23 |
| deryck | sinzui, sure. Looking now.... | 16:24 |
| deryck | sinzui, so I'm combing the permissions now, since that's your main concern.... but one thought, do you need to keep the call to recalculateBugHeatCache in setHeat? Or is doing it in updateHeat enough now? | 16:28 |
| sinzui | SetHeat is still being called on new task, dupe and new bug | 16:31 |
| sinzui | Dup certainly needs to remain | 16:31 |
| deryck | ok, I think it's fine then. | 16:32 |
| bac | hi EdwinGrubbs. i've got a MP that was reviewed by stevenk who is mentored by thumper. i'd really not like to wait until thumper re-appears to land it. would you mind reviewing/mentoring it? | 16:33 |
| deryck | sinzui, the permission changes seem pretty safe to me. Why do you hesitate with them? | 16:36 |
| sinzui | I think it is odd to update distros | 16:37 |
| deryck | yeah, but it's only for heat. You're just worried that allowing update leads to other stuff later without us catching it? | 16:39 |
| deryck | sinzui, is that last statement of mine correct about your concern? ^^ | 16:42 |
| sinzui | deryck, indeed I am paranoid | 16:43 |
| sinzui | deryck, I am confident of our scripts | 16:43 |
| deryck | sinzui, ok. I think your changes are fine. I see your concern, but I think the cost of the paranoia is quite high, and there are other checks to ensure we don't do something we shouldn't here. | 16:44 |
| sinzui | indeed | 16:44 |
| deryck | sinzui, so r=me. Updating MP now... | 16:44 |
| sinzui | thanks | 16:44 |
| deryck | np | 16:45 |
| EdwinGrubbs | bac: sure, what's the url for the mp? | 16:49 |
| bac | EdwinGrubbs: https://code.edge.launchpad.net/~bac/launchpad/bug-618148/+merge/33600 | 16:50 |
| bac | EdwinGrubbs: i'm going to lunch now, though | 16:50 |
| EdwinGrubbs | that's fine | 16:50 |
| benji | jelmer/Edwin: is that an open review slot I see? how about we fill it with https://code.edge.launchpad.net/~benji/launchpad/check-in-wadl/+merge/33661 | 16:54 |
| jelmer | benji: Yes, though I should note I have some concerns about your music suggestions... | 17:01 |
| === jelmer changed the topic of #launchpad-reviews to: On call: jelmer, Edwin || reviewing: benji, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| benji | heh | 17:02 |
| gmb | allenap, I've finally made changes based on your review in https://code.edge.launchpad.net/~gmb/launchpad/dont-piss-the-losas-off-bug-596877/+merge/30186; if you could check that I'm doing TRT that would be great. | 17:03 |
| * allenap looks | 17:03 | |
| jelmer | benji: buildout.cfg has a reference to your homedir :-) | 17:04 |
| benji | jelmer: oh yeah, I was using a not-publicly-available version of lazr.restful; that should affect anything; I'll remove it and double-check | 17:05 |
| benji | s/should/should not/ | 17:06 |
| jelmer | benji: Thanks | 17:06 |
| jelmer | benji: Your MP description mentions tests that compare the stored data to freshly generated data; I can't find them in the branch though, have they already landed? | 17:07 |
| === matsubara-lunch is now known as matsubara | ||
| benji | jelmer: lib/canonical/launchpad/ftests/test_checked_in_wadl.py | 17:09 |
| gmb | allenap, http://bazaar.launchpad.net/~gmb/launchpad/dont-piss-the-losas-off-bug-596877/revision/11154 Specifically; I'll paste a diff that makes it a bit more obvious. | 17:10 |
| jelmer | benji: Sorry, my bad. I just noticed the diff on lp has been truncated. | 17:12 |
| benji | yeah, it was a monster | 17:12 |
| benji | (because of the new, generated files) | 17:12 |
| benji | jelmer: the now-without-a-homedir-reference version has been pushed | 17:19 |
| jelmer | benji: is there a particular reason this needs a newer lazr.restful? | 17:23 |
| allenap | gmb: I've commented. Looks good :) | 17:24 |
| gmb | allenap, Ta. | 17:24 |
| benji | jelmer: yep; the WADL generated by the older lazr.restful wasn't deterministic, so the tests would fail intermittently | 17:24 |
| gmb | allenap, I'll make those changes. Can you give it an r= so that I can land it when I'm done? | 17:25 |
| allenap | gmb: Sure :) | 17:26 |
| gmb | Ta. | 17:29 |
| === Ursinha is now known as Ursinha-lunch | ||
| gmb | allenap, Although, I don't think the last change you suggest is necessary TRT to do; are you okay if I leave that as-is? I don't see us changing that permission (and even if we did, our tests would pick it up). | 17:38 |
| allenap | gmb: Yeah I'm happy :) | 17:39 |
| gmb | Kewl | 17:39 |
| * benji grabs lunch | 17:42 | |
| === benji is now known as benji-lunch | ||
| jelmer | do we allow using the "assert" statement in unit tests as part of the assertions of an individual test? It looks wrong to me, but the style guide doesn't forbid it. | 17:44 |
| gmb | jelmer, I can't answer that without more context (I'm inclined to agree it's not right, but I'd like to see an example) | 17:45 |
| jelmer | gmb: http://bazaar.launchpad.net/~benji/launchpad/check-in-wadl/annotate/head:/lib/canonical/launchpad/ftests/test_checked_in_wadl.py | 17:49 |
| jelmer | gmb: line 87 | 17:49 |
| * gmb looks | 17:50 | |
| benji-lunch | jelmer and gmb: in that case it was an assert that the *code* of the test is correct, much like an assert in "normal" code | 17:50 |
| benji-lunch | I don't really mind changing it to a test assertion but my intent was to communicate that the assert wasn't about the test but about the workings of the test itself | 17:51 |
| gmb | benji-lunch, jelmer: Yeah, that's one that's purely reviewer discretion. Basically, whatever jelmer prefers to see ;) | 17:52 |
| jelmer | benji-lunch: I hadn't looked at it that way; makes sense. | 17:53 |
| jelmer | gmb: Thanks for having a look. | 17:53 |
| gmb | np | 17:53 |
| === salgado is now known as salgado-lunch | ||
| jelmer | abentley: did you notice your mp has conflicts? | 18:16 |
| abentley | jelmer, yes. | 18:16 |
| === jelmer changed the topic of #launchpad-reviews to: On call: Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jelmer | abentley: ok, just checking | 18:17 |
| === benji-lunch is now known as benji | ||
| === Ursinha-lunch is now known as Ursinha | ||
| === sinzui changed the topic of #launchpad-reviews to: On call: Edwin || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| sinzui | EdwinGrubbs, I have a branch ready for review if you have time today. | 18:48 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: bac || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | sinzui: I can do it after I finish bac's branch. | 18:49 |
| sinzui | thanks | 18:49 |
| EdwinGrubbs | bac: it looks like your field.description.replace('pillar', 'project') code is actually changing the IServiceUsage interface, which would be bad if that is ever used for projectgroups and distros. | 19:00 |
| bac | EdwinGrubbs: really? | 19:01 |
| bac | yes, would be bad | 19:01 |
| EdwinGrubbs | bac: you can use pdb to check that, but I believe you would want to use the copy_field() function to get a field you can modify. | 19:02 |
| bac | EdwinGrubbs: i will look in pdb now | 19:02 |
| EdwinGrubbs | bac: it doesn't look like you pushed up the latest changes to your branch. After you're done looking at the field, can you do that? | 19:08 |
| EdwinGrubbs | bac: BTW, I also think that you should change field_names into a @property, so that it is easier to override that if necessary. Right now, setUpFields would overwrite anything that a subclass tried to add to field_names. | 19:09 |
| bac | EdwinGrubbs: ok | 19:10 |
| abentley | rockstar, I can has review? https://code.edge.launchpad.net/~abentley/launchpad/no-chroot-builder/+merge/33674 | 19:14 |
| bac | EdwinGrubbs: you were correct about the interface description being modified in place. good catch. | 19:28 |
| james_w | abentley: with that change it looks like sudo doesn't need to be installed in the chroot anymore? | 19:32 |
| bac | EdwinGrubbs: i've made those changes and updated the MP | 19:32 |
| abentley | james_w, good catch. I'll confirm that. | 19:33 |
| james_w | abentley: though maybe the pbuilder installation of build-deps needs it | 19:33 |
| james_w | I can't see that it does here, but I don't know about all series | 19:34 |
| james_w | I think it expects to be run as root, so we should be ok | 19:34 |
| abentley | james_w, and because the satisfydepends script's requirements aren't considered, the pbuilder package will not have a dependency on sudo... | 19:35 |
| james_w | right, but I don't think that it uses it | 19:35 |
| abentley | james_w, not on lucid, at least. | 19:36 |
| abentley | james_w, I'd like to add a "--safe" option to "dailydeb" that will make it choke on "run" and anything else that could produce arbitrary code execution. What do you think? | 19:38 |
| james_w | abentley: at execution time? | 19:39 |
| abentley | james_w, yes. | 19:39 |
| james_w | abentley: belt and braces? | 19:39 |
| james_w | as I added a way for LP to reject it at parse time as well | 19:40 |
| abentley | james_w, exactly. | 19:40 |
| james_w | I'm fine with that | 19:40 |
| abentley | james_w, ? we already reject it at parse time. | 19:40 |
| james_w | abentley: yes, but this was at rockstar's request, to do it in a cleaner fashion | 19:42 |
| abentley | james_w, oh, cool. | 19:42 |
| james_w | not meaning to say that you didn't, just making sure it was in addition, not instead of | 19:43 |
| james_w | --safe will be easy to add though | 19:43 |
| james_w | you can file a bug and I will get to it eventually, or you could add it easily if you like | 19:43 |
| EdwinGrubbs | bac: r=me | 19:44 |
| bac | thanks EdwinGrubbs | 19:46 |
| abentley | james_w, I would prefer a list of permitted instructions instead of a list of forbidden instructions. You fail safe that way. | 19:50 |
| === salgado-lunch is now known as salgado | ||
| james_w | abentley: true, but requires more work for LP :-) | 19:52 |
| james_w | easy enough to change | 19:52 |
| bac | EdwinGrubbs: could i get you to mark the MP? | 19:52 |
| james_w | abentley: bzr says that sudo has never been used by a satisfydepends script | 19:52 |
| EdwinGrubbs | bac: sure, I was just writing up some comments for Steve also. | 19:53 |
| bac | EdwinGrubbs: great | 19:53 |
| abentley | james_w, you mean, except for satisfydepends-experimental? | 19:54 |
| james_w | abentley: in a comment? | 19:55 |
| abentley | james_w, oh, didn't see that. Cool. | 19:55 |
| abentley | james_w, builder trunk has a test failure: http://pastebin.ubuntu.com/483595/ | 20:08 |
| james_w | abentley: hmm, that depends on the version of python-debian apparently | 20:09 |
| james_w | abentley: maybe make it .startswith, or generate that exception ourselves and put it in to the string we are asserting against? | 20:09 |
| abentley | james_w, if that message is externally-generated, I think it makes sense to leave it alone. | 20:11 |
| EdwinGrubbs | sinzui: do you want me to review official-services or mailing-list-subscribers-cache first? | 20:12 |
| james_w | well, the test was to check that it included the str(e) of the exception from python-debian, but it's brittle as written. I don't think it's too important, so either way of avoiding hardcoding it is fine by me. | 20:12 |
| sinzui | official-services plaase | 20:12 |
| bac | EdwinGrubbs: time for a super-easy one? | 20:46 |
| bac | sinzui: or, perhaps you could look at it? https://code.edge.launchpad.net/~bac/launchpad/bug-429248/+merge/33684 | 20:47 |
| EdwinGrubbs | that would be better, since I should review sinzui's first, although he doesn't seem to be around. | 20:47 |
| sinzui | I am | 20:48 |
| sinzui | EdwinGrubbs, oh, I forgot to include your name in my answer. official-services is the one I really want reviewed first | 20:49 |
| EdwinGrubbs | oh | 20:49 |
| bac | sinzui: can you glance at my branch? | 20:50 |
| sinzui | yep | 20:50 |
| bac | thanks | 20:50 |
| sinzui | we could trade, I have a trival fix too | 20:51 |
| sinzui | trivial cache | 20:51 |
| bac | sinzui: gladly | 20:51 |
| sinzui | bac: https://code.launchpad.net/~sinzui/launchpad/mailing-list-subscribers-cache-0/+merge/33681 | 20:52 |
| sinzui | bac: r=me | 20:53 |
| bac | sinzui: that's not fair. i'm still reading your description. | 20:53 |
| sinzui | the diff is shorter too | 20:53 |
| bac | only b/c i'm so conscientious and fixed the lint stuff | 20:56 |
| benji | jelmer: I just noted on https://code.edge.launchpad.net/~benji/launchpad/check-in-wadl/+merge/33661 that your two review requests have been done. | 21:35 |
| benji | mmm, I think jelmer's not home any more | 21:35 |
| EdwinGrubbs | sinzui: I don't see why you create the selected_template property that returns self.template instead of just turning self.template into a property. That should eliminate the need to define render(). | 21:38 |
| sinzui | EdwinGrubbs, yes. I considered that. I decided that most engineers are not aware of that. That take render for granted. I decided to create selected_template to be clear that something special is going on | 21:39 |
| sinzui | s/that take/they take/ | 21:39 |
| sinzui | EdwinGrubbs, by example, when XRDS breaks the person page, I am the person who has to explain how...it is not obvious that render() is redefined | 21:40 |
| EdwinGrubbs | sinzui: ok, then I think you should rename self.template, so that programmers who are aware of that aren't confused that it is not being used implicitly by render(). | 21:42 |
| EdwinGrubbs | sinzui: I can't remember what XRDS stands for. | 21:42 |
| sinzui | me neither | 21:42 |
| sinzui | self.template cannot be a property (I tried it). The call modifies the base class in an od way | 21:43 |
| benji | "eXtensible Resource Descriptor Sequence"? | 21:43 |
| sinzui | EdwinGrubbs, the choice was to modify render() to do the work quietly or try a more explicit approach | 21:44 |
| sinzui | EdwinGrubbs, If you really want, I can make render do the work and add comments warning future developers to that they need to understand the base class | 21:45 |
| EdwinGrubbs | sinzui: no, I don't want you to move the logic into render(). It is just suprising to see selected_template used like self.template, and self.template not used like template normallly is. | 21:47 |
| * sinzui nods | 21:47 | |
| jcsackett | EdwinGrubbs: are you going to have time for another review today (it's pretty big) or should i just hunt someone down tomorrow? | 21:48 |
| EdwinGrubbs | jcsackett: I might be able to start on it today. | 21:48 |
| sinzui | template cannot be a property so I chose selected template. I can add a comment about the decision in selected_template | 21:48 |
| jcsackett | EdwinGrubbs: okay, i'll add myself to the queue. | 21:48 |
| === jcsackett changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: bac || queue: [sinzui, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | sinzui: really? Why can't template be a property? | 21:49 |
| jcsackett | EdwinGrubbs: https://code.edge.launchpad.net/~jcsackett/launchpad/deprecate-official_malone/+merge/33694 | 21:50 |
| sinzui | Calling ViewPageTemplateFile does magic | 21:50 |
| jcsackett | if you can get to it, thanks in advance. :-) | 21:50 |
| sinzui | EdwinGrubbs, ^ | 21:50 |
| sinzui | Lots of tests broken when made a property of it. It seemed like the obvious course at the time. And a nasty surprise afterwards | 21:51 |
| === benji changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: bac || queue: [sinzui, jcsackett, benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | sinzui: I was confused by what ViewPageTemplateFile would be doing. I tried looking at the source code, which is a mess, so I just made the changes to see what error it would give. All the tests passed with SearchQuestionsView.template as a property. | 22:07 |
| sinzui | I am confused | 22:08 |
| sinzui | I will certainly do that if all answers passes | 22:08 |
| EdwinGrubbs | bac: ping | 22:30 |
| bac | EdwinGrubbs: yes? | 22:30 |
| EdwinGrubbs | bac: am I remembering correctly that all lists should be formatted vertically? Not just the recent import statement change. | 22:31 |
| bac | EdwinGrubbs: yes | 22:31 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || reviewing: bac || queue: [sinzui, jcsackett, benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| EdwinGrubbs | sinzui: review sent | 22:41 |
| sinzui | Thanks EdwinGrubbs | 22:41 |
| === benji changed the topic of #launchpad-reviews to: On call: - || reviewing: bac || queue: [sinzui, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === Ursinha is now known as Ursinha-afk | ||
| === matsubara is now known as matsubara-afk | ||
| === Guest83593 is now known as jelmer | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!