| === sidnei_ is now known as sidnei | ||
| === mwhudson_ is now known as mwhudson | ||
| === mwhudson_ is now known as mwhudson | ||
| === henninge_ is now known as henninge | ||
| jtv | on call: jtv (limited availabality) || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ | 09:36 |
|---|---|---|
| === jtv changed the topic of #launchpad-reviews to: on call: jtv (limited availabality) || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| BjornT_ | jtv: time for a quick review? i'd like to get it merged before going to lunch. https://code.edge.launchpad.net/~bjornt/launchpad/bug-462502/+merge/14082 | 11:13 |
| jtv | BjornT_: sure, I think I owe you that much. :) | 11:13 |
| jtv | BjornT_: looks like you made the line breaking a bit saner as well... | 11:18 |
| jtv | BjornT_: I know you inherited this, but why $(shell $(shell pwd)/foo) instead of $(shell ./foo) ? | 11:21 |
| jtv | I guess the outer shell invocation can run from multiple directories... | 11:23 |
| jtv | BjornT_: is there a way to verify that all of this doesn't regress? | 11:23 |
| BjornT_ | jtv: no idea. i prefer not to change it, though, just in case it would break anything. | 11:23 |
| BjornT_ | jtv: well, the jscheck builder should catch errors like these | 11:24 |
| BjornT_ | jtv: it doesn't do a good job atm, but that's actually how i found out. i want to fix the 3 failing tests, and noticed that suddenly all the tests were failing | 11:24 |
| jtv | even in devmode? | 11:24 |
| BjornT_ | jtv: windmill doesn't run in devmode | 11:25 |
| BjornT_ | and in devmode, this code isn't used | 11:25 |
| jtv | BjornT_: then I guess this is tested about as well as it can be at the moment | 11:26 |
| BjornT_ | jtv: yeah, i can't think of a better way of testing it. we need to actually evaluate the JS to see that it's sane. maybe jslint would work. anyway, we actually have failing tests that will get fixed by this patch. | 11:28 |
| jtv | FWIW ISTM that EXTRA_JS_FILES lost its meaning when yui was added the way it was... why not insert yui in there? | 11:29 |
| === matsubara-afk is now known as matsubara | ||
| BjornT_ | jtv: we need to execute a script to find out which yui files to include. before executing the script, we need to run the jscheck_lazr step | 11:30 |
| BjornT_ | jtv: so, if it's possible, we could define EXTRA_JS_FILES in the jsbuild target, but it seems a bit pointless | 11:31 |
| BjornT_ | s/jscheck/jsbuild/ | 11:31 |
| jtv | BjornT_: sure, no worries. That variable doesn't look like it ever really pulled its weight. | 11:31 |
| BjornT_ | indeed. it was only used in one place | 11:32 |
| jtv | BjornT_: grep confirms that. :) r=jtv, enjoy your lunch. | 11:32 |
| BjornT_ | jtv: thanks! | 11:33 |
| jtv | np | 11:33 |
| === Ursinha-afk is now known as Ursinha | ||
| === henninge changed the topic of #launchpad-reviews to: on call: jtv (limited availabality) || reviewing: - || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| henninge | jtv: can you review my branch after the call? | 12:01 |
| === jtv changed the topic of #launchpad-reviews to: on call: jtv (limited availability) || reviewing: - || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| jtv | henninge: you can make it really boring so I can review during the call. | 12:01 |
| henninge | ;_) | 12:01 |
| henninge | jtv: what's the "from_sourcepackage" thing? | 12:15 |
| === mrevell is now known as mrevell-lunch | ||
| === mrevell-lunch is now known as mrevell | ||
| henninge | jtv: MP's away. | 12:45 |
| jtv | henninge: get to it in a bit | 12:45 |
| henninge | jtv: endulge yourself while I go for lunch ... ;-) | 12:45 |
| === henninge is now known as henninge-lunch | ||
| * jtv indulges self | 12:45 | |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: on call: jtv (limited availability), Edwin || reviewing: - || queue: [henninge] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| jtv | EdwinGrubbs: I promised to do Henning's, so I'll get on that. | 13:37 |
| === henninge-lunch is now known as henninge | ||
| henninge | jtatum: I am back | 13:50 |
| henninge | jtv: ^ | 13:50 |
| jtv | henninge: stop bothering that poor person! | 13:50 |
| jtv | I'm reviewing your branch now | 13:50 |
| jtv | one small note: you shouldn't have to sqlvalues(sourcepackagename.id); just sqlvalues(sourcepackagename) or quote(sourcepackagename) should do. | 13:51 |
| === flacoste_afk is now known as flacoste | ||
| jtv | henninge: also, the ordered/just_for_count dichotomy is awful. As I read the code, won't prejoin just delegate count() to the original result set anyway? | 14:05 |
| jtv | henninge: if it does, there's no need to special-case the count queries. | 14:06 |
| henninge | jtv: the comment says that count performs and unnecessary join that makes it slower, so I avoid this, I guess. | 14:08 |
| henninge | jtv: But I am really not sure about the performance implications and that's all this about. | 14:08 |
| jtv | henninge: which comment? | 14:09 |
| henninge | in prejoin.py | 14:09 |
| henninge | probably not part of the diff | 14:09 |
| jtv | ah, see it | 14:09 |
| jtv | more importantly, the syntax of the new query confuses me. Specifically, is it still really a left join? It looks like you're inner-joining SourcePackageName. | 14:11 |
| jtv | ah, you only construct that prejoin if you're looking for distro templates. | 14:13 |
| jtv | henninge: the "Select all source package names in a distribution" comment isn't quite correct | 14:17 |
| henninge | jtv: no, that should be "Select all POTemplates ..." | 14:18 |
| jtv | and ...in a distroseries. | 14:19 |
| henninge | oh, right | 14:19 |
| jtv | henninge: how about renaming just_for_count to prefetch, and inverting its meaning? | 14:21 |
| jtv | (The from_sourcepackagename refactoring looks semantically neutral to me, so that's fine I think) | 14:21 |
| henninge | jtv: do_prefetch ? | 14:22 |
| jtv | henninge: if you like, sure | 14:22 |
| henninge | ok | 14:22 |
| jtv | I'm a bit surprised that all these SQL conditions are still in strings even though you converted the query construction to Storm... care to have a bash at that? | 14:22 |
| jtv | self.clauses.append(POTemplate.distroseriesID == distroseries.id) etc. | 14:23 |
| jtv | henninge: ^^^ | 14:26 |
| jtv | You can just And(*self.clauses) instead of " AND ".join(clauses). | 14:27 |
| henninge | where's that ? | 14:27 |
| jtv | henninge: in POTemplateSubset | 14:28 |
| henninge | jtv: I am sorry, I cannot find "AND" in POTemplateSubset. Gotta line number? | 14:30 |
| henninge | jtv: what is the meaning of the "||" in this: http://paste.ubuntu.com/303592/ | 14:42 |
| henninge | jtv: is it just concatenation? | 14:42 |
| jtv | That is SQL's concatenation operator, yes | 14:43 |
| jtv | henninge: I'll be having to leave very soon. The branch is OK for me, with the changes we discussed; converting the conditions to Storm is needed only if it doesn't take too much time. | 14:48 |
| henninge | jtv: I am as good as done but I still don't see where " AND " is used ... | 14:49 |
| jtv | henninge: then it probably isn't. | 14:49 |
| henninge | jtv: here is the incremental patch: http://paste.ubuntu.com/303597/ | 14:51 |
| === jtv changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| jtv | henninge: great, r=me | 14:51 |
| * jtv eods | 14:51 | |
| sinzui | EdwinGrubbs: I just reviewed you CSS fix for action-menus | 15:33 |
| EdwinGrubbs | thanks | 15:34 |
| EdwinGrubbs | sinzui: what browser did you use that gave you the clipped icons? | 15:43 |
| sinzui | firefox | 15:43 |
| === salgado is now known as salgado-lunch | ||
| abentley | EdwinGrubbs: Could you please review https://code.launchpad.net/~abentley/launchpad/prerequisite-diff/+merge/14092 ? | 16:02 |
| === deryck is now known as deryck[lunch] | ||
| EdwinGrubbs | abentley: sure | 16:18 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| abentley | EdwinGrubbs: Thanks. | 16:18 |
| === beuno is now known as beuno-lunch | ||
| === Ursinha is now known as Ursinha-afk | ||
| === beuno-lunch is now known as beuno | ||
| === rockstar is now known as rockstar-afk | ||
| === deryck[lunch] is now known as deryck | ||
| === salgado-lunch is now known as salgado | ||
| === leonardr_ is now known as leonardr | ||
| sinzui | abentley: r=me | 17:32 |
| abentley | sinzui: Thanks. | 17:32 |
| === matsubara is now known as matsubara-lunch | ||
| === abentley is now known as abentley-lunch | ||
| intellectronica | EdwinGrubbs: can you review a small branch for me? it fixes the oops i created on project pages | 18:03 |
| intellectronica | sinzui: maybe you want to review it? it's very small, a fix for the oops you showed me earlier | 18:05 |
| sinzui | intellectronica: I would like to | 18:06 |
| intellectronica | sinzui: thanks. https://code.edge.launchpad.net/~intellectronica/launchpad/oops-462742/+merge/14104 | 18:06 |
| intellectronica | sinzui: any questions about my patch? | 18:15 |
| sinzui | intellectronica: sorry, I had my turn to speak on a conference call. | 18:17 |
| * sinzui focuses on many things | 18:17 | |
| intellectronica | ah sorry, i can find someone else to review this, i'm sure you've got more than enough | 18:17 |
| sinzui | intellectronica: reading the test, I think the reason we did not see this before because it only happens when there is a structural subscription | 18:21 |
| intellectronica | sinzui: yes, the test makes sure there is one | 18:22 |
| sinzui | intellectronica: the error really happened when <menu>.subscribe was called, This test will pass if the the template omits the link for anonymous users or if the link is decorated with @requires('launchpad.View') | 18:28 |
| === rockstar-afk is now known as rockstar | ||
| intellectronica | sinzui: no, the error happens when the link is rendered, and this test is (i'm pretty sure) the only possible one | 18:29 |
| sinzui | intellectronica: okay | 18:30 |
| sinzui | r=me | 18:30 |
| intellectronica | sinzui: danke! | 18:30 |
| intellectronica | sinzui: can you plz also mark https://code.edge.launchpad.net/~intellectronica/launchpad/oops-462742/+merge/14104 | 18:30 |
| intellectronica | ah you just did | 18:31 |
| sinzui | intellectronica: yes, i am just slow at clicking since I am interested in the conversation I am listening to. | 18:31 |
| === abentley-lunch is now known as abentley | ||
| abentley | EdwinGrubbs: How's it going? | 18:32 |
| EdwinGrubbs | abentley: fine, I just finished it. r=me | 18:33 |
| abentley | EdwinGrubbs: Thanks! | 18:33 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| === matsubara-lunch is now known as matsubara | ||
| === Ursinha-afk is now known as Ursinha | ||
| === salgado is now known as salgado-afk | ||
| leonardr | edwin, can you take a look at https://code.edge.launchpad.net/~leonardr/launchpadlib/trusted-workflow/+merge/14107 ? | 20:16 |
| leonardr | i'm happy to explain the code in more detail if oyu find it confusing, but take a look at trusted-client.txt first | 20:16 |
| EdwinGrubbs | leonardr: sure | 20:17 |
| EdwinGrubbs | leonardr: does ScriptableRequestTokenAuthorization have a UI, or is it just a library? | 20:34 |
| leonardr | Edwin: it is just a library. in a future branch i will give it a command-line ui, and other people will give it other guis | 20:35 |
| EdwinGrubbs | leonardr: can user_chose_other_than_unauthorized() not be called user_chose_authorized()? | 20:46 |
| leonardr | edwin, that implies that 'authorized' is a specific level of access | 20:52 |
| leonardr | it could be user_authorized vs. user_refused_to_authorize | 20:53 |
| leonardr | actually i like that better | 20:55 |
| === matsubara is now known as matsubara-afk | ||
| EdwinGrubbs | leonardr: when I try to run the tests, it completely ignores the egg for lazr.restfulclient. Have you seen that before? | 21:25 |
| leonardr | edwin: are you running the tests with bin/test in launchpadlib, or are you running them through a launchpad install? | 21:26 |
| leonardr | to answer your question, no, i'm not sure what you're talking about. why does it matter what restfulclient egg is used? | 21:27 |
| EdwinGrubbs | leonardr: it's using the restfulclient that's installed on the system, and it doesn't have lazr.restfuleclient.authorize.oauth. | 21:32 |
| leonardr | hmm | 21:34 |
| leonardr | well, setup.py requires at least 0.9.9 | 21:34 |
| leonardr | so if you ran buildout it should be using 0.9.9 | 21:34 |
| leonardr | again the question: are you getting this error running bin/test in launchpadlib or in launchpad? | 21:35 |
| EdwinGrubbs | leonardr: I get the error when running ./bin/test in launchpadlib. | 21:35 |
| leonardr | ok, to run the tests that are new in this branch you'll need to hook up this launchpadlib to a launchpad installation and run them with bin/test -vvt launchpadlib | 21:36 |
| leonardr | because these tests can only be run against a working launchpad | 21:37 |
| leonardr | but, i don't know why bootstrap.py should let you have an old version of lazr.restfulclient | 21:37 |
| leonardr | what version is it using? 0.9.5? | 21:37 |
| leonardr | i'd run bootstrap and see whether it mentions restfulclient. if not, hack setup.py to require an impossible version of restfulclient and see if it complains then | 21:38 |
| EdwinGrubbs | leonardr: it's running 0.9.3 from site-packages, and I can't seem to get it to ignore that. | 21:42 |
| leonardr | buildout doesn't complain if you make setup.py demand eg. version 5.5.5 of restfulclient? | 21:43 |
| EdwinGrubbs | leonardr: After I removed the python-lazr-restfulclient and the python-lazr-uri packages, so that the /usr/lib/python2.4/site-packages/lazr directory doesn't even exist, then it found the egg that was already on its sys.path. | 21:55 |
| leonardr | edwingrubbs: ah, i don't have those packages installed. i probably removed them so i could do dev work | 21:55 |
| EdwinGrubbs | leonardr: it's a little crazy, since both apport and ubuntuone need those packages. | 21:56 |
| leonardr | edwingrubbs: gary might have an idea of how to get this to work with those packages installed | 21:57 |
| gary_poster | EdwinGrubbs: grr. That problem is supposed to be smashed. this is on devel? | 21:58 |
| gary_poster | EdwinGrubbs: oh | 21:58 |
| gary_poster | EdwinGrubbs: this is when you are developing launchpadlib, not launchpad, isn't it? | 21:59 |
| EdwinGrubbs | gary: right | 21:59 |
| EdwinGrubbs | gary_poster: It seems kind of evil that apport is imported in both sitecustomize.py and site.py, which could be bringing in lazr before the sys.path is modified. | 22:00 |
| gary_poster | EdwinGrubbs: ok. yeah, it is evil and nasty, but this class of problem is fixed in the buildout branch we use in Launchpad. Jim Fulton has been supposed to review and merge my zc.buildout branches for maybe months now. :-/ If I were you, I would use a clean Python while developing non-launchpad stuff that uses buildout. A similar solution is that you can use a virtualenv Python with --no-site-packages. | 22:01 |
| thumper | EdwinGrubbs: still reviewing? | 22:37 |
| thumper | https://code.edge.launchpad.net/~thumper/launchpad/fix-commit-message-oops/+merge/14120 | 22:37 |
| EdwinGrubbs | thumper: sure, I can take one more. | 22:37 |
| thumper | EdwinGrubbs: it is really simple | 22:38 |
| EdwinGrubbs | thumper: r=me | 22:45 |
| thumper | EdwinGrubbs: ta | 22:45 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ | ||
| thumper | mwhudson: https://code.edge.launchpad.net/~thumper/launchpad/fix-vocab-oops/+merge/14065 diff now updated | 22:59 |
| mwhudson | thumper: +1 | 23:00 |
| mwhudson | (with a side of "oh god, getByLPPath is insane") | 23:00 |
| thumper | mwhudson: ta | 23:00 |
| thumper | mwhudson: oh, and thanks again for making --headless much much faster | 23:01 |
| * thumper does ec2 land twice | 23:01 | |
| mwhudson | thumper: np | 23:01 |
| mwhudson | thumper: i guess it might be time to make a new image come to think of it... | 23:01 |
| mwhudson | anyone for a really easy review? rockstar ? | 23:49 |
| rockstar | mwhudson, I'll show you mine if you show me yours. | 23:49 |
| mwhudson | rockstar: thanks, sent | 23:53 |
| === Ursinha is now known as Ursinha-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!