=== sidnei_ is now known as sidnei
=== mwhudson_ is now known as mwhudson
=== mwhudson_ is now known as mwhudson
=== henninge_ is now known as henninge
jtvon 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/1408211:13
jtvBjornT_: sure, I think I owe you that much.  :)11:13
jtvBjornT_: looks like you made the line breaking a bit saner as well...11:18
jtvBjornT_: I know you inherited this, but why $(shell $(shell pwd)/foo) instead of $(shell ./foo) ?11:21
jtvI guess the outer shell invocation can run from multiple directories...11:23
jtvBjornT_: 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 these11: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 failing11:24
jtveven in devmode?11:24
BjornT_jtv: windmill doesn't run in devmode11:25
BjornT_and in devmode, this code isn't used11:25
jtvBjornT_: then I guess this is tested about as well as it can be at the moment11: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
jtvFWIW 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 step11:30
BjornT_jtv: so, if it's possible, we could define EXTRA_JS_FILES in the jsbuild target, but it seems a bit pointless11:31
jtvBjornT_: 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 place11:32
jtvBjornT_: grep confirms that.  :)  r=jtv, enjoy your lunch.11:32
BjornT_jtv: thanks!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/
henningejtv: 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/
jtvhenninge: you can make it really boring so I can review during the call.12:01
henningejtv: what's the "from_sourcepackage" thing?12:15
=== mrevell is now known as mrevell-lunch
=== mrevell-lunch is now known as mrevell
henningejtv: MP's away.12:45
jtvhenninge: get to it in a bit12:45
henningejtv: endulge yourself while I go for lunch ... ;-)12:45
=== henninge is now known as henninge-lunch
* jtv indulges self12: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/
jtvEdwinGrubbs: I promised to do Henning's, so I'll get on that.13:37
=== henninge-lunch is now known as henninge
henningejtatum: I am back13:50
henningejtv: ^13:50
jtvhenninge: stop bothering that poor person!13:50
jtvI'm reviewing your branch now13:50
jtvone 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
jtvhenninge: 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
jtvhenninge: if it does, there's no need to special-case the count queries.14:06
henningejtv: the comment says that count performs and unnecessary join that makes it slower, so I avoid this, I guess.14:08
henningejtv: But I am really not sure about the performance implications and that's all this about.14:08
jtvhenninge: which comment?14:09
henningein prejoin.py14:09
henningeprobably not part of the diff14:09
jtvah, see it14:09
jtvmore 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
jtvah, you only construct that prejoin if you're looking for distro templates.14:13
jtvhenninge: the "Select all source package names in a distribution" comment isn't quite correct14:17
henningejtv: no, that should be "Select all POTemplates ..."14:18
jtvand ...in a distroseries.14:19
henningeoh, right14:19
jtvhenninge: 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
henningejtv: do_prefetch ?14:22
jtvhenninge: if you like, sure14:22
jtvI'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
jtvself.clauses.append(POTemplate.distroseriesID == distroseries.id) etc.14:23
jtvhenninge: ^^^14:26
jtvYou can just And(*self.clauses) instead of " AND ".join(clauses).14:27
henningewhere's that ?14:27
jtvhenninge: in POTemplateSubset14:28
henningejtv: I am sorry, I cannot find "AND" in POTemplateSubset. Gotta line number?14:30
henningejtv: what is the meaning of the "||" in this: http://paste.ubuntu.com/303592/14:42
henningejtv: is it just concatenation?14:42
jtvThat is SQL's concatenation operator, yes14:43
jtvhenninge: 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
henningejtv: I am as good as done but I still don't see where " AND " is used ...14:49
jtvhenninge: then it probably isn't.14:49
henningejtv: 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/
jtvhenninge: great, r=me14:51
* jtv eods14:51
sinzuiEdwinGrubbs: I just reviewed you CSS fix for action-menus15:33
EdwinGrubbssinzui: what browser did you use that gave you the clipped icons?15:43
=== salgado is now known as salgado-lunch
abentleyEdwinGrubbs: Could you please review https://code.launchpad.net/~abentley/launchpad/prerequisite-diff/+merge/14092 ?16:02
=== deryck is now known as deryck[lunch]
EdwinGrubbsabentley: sure16:18
=== EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
abentleyEdwinGrubbs: 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
sinzuiabentley: r=me17:32
abentleysinzui: Thanks.17:32
=== matsubara is now known as matsubara-lunch
=== abentley is now known as abentley-lunch
intellectronicaEdwinGrubbs: can you review a small branch for me? it fixes the oops i created on project pages18:03
intellectronicasinzui: maybe you want to review it? it's very small, a fix for the oops you showed me earlier18:05
sinzuiintellectronica: I would like to18:06
intellectronicasinzui: thanks. https://code.edge.launchpad.net/~intellectronica/launchpad/oops-462742/+merge/1410418:06
intellectronicasinzui: any questions about my patch?18:15
sinzuiintellectronica: sorry, I had my turn to speak on a conference call.18:17
* sinzui focuses on many things18:17
intellectronicaah sorry, i can find someone else to review this, i'm sure you've got more than enough18:17
sinzuiintellectronica: reading the test, I think the reason we did not see this before because it only happens when there is a structural subscription18:21
intellectronicasinzui: yes, the test makes sure there is one18:22
sinzuiintellectronica: 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
intellectronicasinzui: no, the error happens when the link is rendered, and this test is (i'm pretty sure) the only possible one18:29
sinzuiintellectronica: okay18:30
intellectronicasinzui: danke!18:30
intellectronicasinzui: can you plz also mark https://code.edge.launchpad.net/~intellectronica/launchpad/oops-462742/+merge/1410418:30
intellectronicaah you just did18:31
sinzuiintellectronica: 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
abentleyEdwinGrubbs: How's it going?18:32
EdwinGrubbsabentley: fine, I just finished it. r=me18:33
abentleyEdwinGrubbs: 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
leonardredwin, can you take a look at https://code.edge.launchpad.net/~leonardr/launchpadlib/trusted-workflow/+merge/14107 ?20:16
leonardri'm happy to explain the code in more detail if oyu find it confusing, but take a look at trusted-client.txt first20:16
EdwinGrubbsleonardr: sure20:17
EdwinGrubbsleonardr: does ScriptableRequestTokenAuthorization have a UI, or is it just a library?20:34
leonardrEdwin: it is just a library. in a future branch i will give it a command-line ui, and other people will give it other guis20:35
EdwinGrubbsleonardr: can user_chose_other_than_unauthorized() not be called user_chose_authorized()?20:46
leonardredwin, that implies that 'authorized' is a specific level of access20:52
leonardrit could be user_authorized vs. user_refused_to_authorize20:53
leonardractually i like that better20:55
=== matsubara is now known as matsubara-afk
EdwinGrubbsleonardr: when I try to run the tests, it completely ignores the egg for lazr.restfulclient. Have you seen that before?21:25
leonardredwin: are you running the tests with bin/test in launchpadlib, or are you running them through a launchpad install?21:26
leonardrto answer your question, no, i'm not sure what you're talking about. why does it matter what restfulclient egg is used?21:27
EdwinGrubbsleonardr: it's using the restfulclient that's installed on the system, and it doesn't have lazr.restfuleclient.authorize.oauth.21:32
leonardrwell, setup.py requires at least 0.9.921:34
leonardrso if you ran buildout it should be using 0.9.921:34
leonardragain the question: are you getting this error running bin/test in launchpadlib or in launchpad?21:35
EdwinGrubbsleonardr: I get the error when running ./bin/test in launchpadlib.21:35
leonardrok, 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 launchpadlib21:36
leonardrbecause these tests can only be run against a working launchpad21:37
leonardrbut, i don't know why bootstrap.py should let you have an old version of lazr.restfulclient21:37
leonardrwhat version is it using? 0.9.5?21:37
leonardri'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 then21:38
EdwinGrubbsleonardr: it's running 0.9.3 from site-packages, and I can't seem to get it to ignore that.21:42
leonardrbuildout doesn't complain if you make setup.py demand eg. version 5.5.5 of restfulclient?21:43
EdwinGrubbsleonardr: 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
leonardredwingrubbs: ah, i don't have those packages installed. i probably removed them so i could do dev work21:55
EdwinGrubbsleonardr: it's a little crazy, since both apport and ubuntuone need those packages.21:56
leonardredwingrubbs: gary might have an idea of how to get this to work with those packages installed21:57
gary_posterEdwinGrubbs: grr.  That problem is supposed to be smashed.  this is on devel?21:58
gary_posterEdwinGrubbs: oh21:58
gary_posterEdwinGrubbs: this is when you are developing launchpadlib, not launchpad, isn't it?21:59
EdwinGrubbsgary: right21:59
EdwinGrubbsgary_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_posterEdwinGrubbs: 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
thumperEdwinGrubbs: still reviewing?22:37
EdwinGrubbsthumper: sure, I can take one more.22:37
thumperEdwinGrubbs: it is really simple22:38
EdwinGrubbsthumper: r=me22:45
thumperEdwinGrubbs: ta22:45
=== EdwinGrubbs changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/
thumpermwhudson: https://code.edge.launchpad.net/~thumper/launchpad/fix-vocab-oops/+merge/14065 diff now updated22:59
mwhudsonthumper: +123:00
mwhudson(with a side of "oh god, getByLPPath is insane")23:00
thumpermwhudson: ta23:00
thumpermwhudson: oh, and thanks again for making --headless much much faster23:01
* thumper does ec2 land twice23:01
mwhudsonthumper: np23:01
mwhudsonthumper: i guess it might be time to make a new image come to think of it...23:01
mwhudsonanyone for a really easy review?  rockstar ?23:49
rockstarmwhudson, I'll show you mine if you show me yours.23:49
mwhudsonrockstar: thanks, sent23:53
=== Ursinha is now known as Ursinha-afk

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!