[01:26] <mwhudson> thumper: can you review https://code.edge.launchpad.net/~mwhudson/launchpad/reduce-concurrent-job-count/+merge/19574 pls?
[01:27]  * thumper looks
[01:30] <thumper> mwhudson: are you happy with the value 3?
[01:30] <thumper> mwhudson: I'm wondering what we'll do with a new quad core machine soon
[01:30] <mwhudson> thumper: yeah, that did occur to me too
[01:31] <thumper> mwhudson: anyway, happy for this to go in.
[01:31] <thumper> r=me
[01:31] <mwhudson> thumper: maybe i should change it to be a paramter to getJobsForMachine, and have the slave pass it in
[01:31] <mwhudson> that's much easier to change via cp
[01:31] <thumper> yeah
[01:32] <thumper> can we have it as a command line param?
[01:32] <thumper> or at least have a command line override the default if supplied
[01:32] <thumper> that way we can keep this config variable
[01:32]  * thumper runs to collect Rachel and a coffee
[02:24] <mwhudson> thumper: want to review incremental-code-imports-bug-512683 ?
[02:24] <thumper> mwhudson: sure
[02:24]  * mwhudson boggles
[02:24] <mwhudson> i can't type new lines into this textarea
[02:26]  * mwhudson restarts ff, it works now
[02:26] <mwhudson> stupid free software crap
[02:27] <mwhudson> <wink>
[02:40] <mwhudson> thumper: you have mail
[02:55] <thumper> ok
[02:55] <thumper> mwhudson: damn out of order emails
[02:56] <mwhudson> thumper: :-)
[02:56] <thumper> mwhudson: is 5k too much?
[02:56] <thumper> mwhudson: how many on the kernel before it goes slow?
[02:56] <mwhudson> thumper: ot
[02:56] <mwhudson> it's a bit of a guess, to be sure
[02:56]  * mwhudson goes to look at some log files
[02:58] <mwhudson> it probably is too many, takes about 12 hours for the kernel?
[02:59] <mwhudson> 2500 took about 6 hours
[02:59] <thumper> 1000?
[02:59] <mwhudson> 1000 about 1hr 30
[02:59] <thumper> lets use that
[03:00] <mwhudson> thumper: ok
[03:00] <thumper> mwhudson: I don't have an issue with an intial git import taking 20 or 30 partial successes
[03:00] <thumper> mwhudson: is this mainline revisions or total revisions?
[03:00] <mwhudson> thumper: total
[03:00] <mwhudson> thumper: linux is 168 k :-)
[03:01] <thumper> hmm...
[03:01] <thumper> well, worth a crack nigel
[03:01] <mwhudson> it'll be easy to chage with a cowboy
[03:02] <mwhudson> you could even do a time based limit i guess
[03:03] <mwhudson> though that'd require changing bzr-git again
[03:04] <thumper> mwhudson:  in test_worker don't you want TestCaseWithFactory rather than just TestCase?
[03:05] <mwhudson> thumper: no, these tests don't interact with the database
[03:05] <thumper> mwhudson: ok
[03:05] <mwhudson> thumper: small changes pushed btw
[03:05] <mwhudson> (i forgot to commit some self-review)
=== _thumper_ is now known as thumper
[03:32] <mwhudson> thumper: thanks for the review
[03:32] <thumper> mwhudson: np
[03:32] <thumper> it is Friday afternoon
[03:33] <mwhudson> thumper: do you think we should get a UI review?
[03:33] <thumper> short answer: no
[03:33] <mwhudson> ok
[03:33] <thumper> we may want to get someone to look at it later
[03:33] <thumper> but we shouldn't block on it
[03:33] <mwhudson> yeah ok
[03:33] <thumper> the actual ui change is almost unnoticable
[03:34] <thumper> and only visible in certain early circumstances
[03:34] <thumper> noticable by us, but probably not too many others
[03:34]  * mwhudson ec2 lands
[03:36] <mwhudson> thumper: can you look at https://code.edge.launchpad.net/~mwhudson/launchpad/reduce-concurrent-job-count/+merge/19574 again?
[03:36] <thumper> ok
[03:42] <mwhudson> ta
[03:47] <thumper> mwhudson: email coming your way
[03:49] <mwhudson> heh, that's what i get for not writing tests i guess
[03:50] <mwhudson> *running
[03:50] <mwhudson> (also coding on a friday afternoon)
[03:59] <thumper> https://devpad.canonical.com/~tim/description1.png
[03:59] <thumper> https://devpad.canonical.com/~tim/description2.png
[03:59] <thumper> https://devpad.canonical.com/~tim/description3.png
[03:59] <thumper> https://devpad.canonical.com/~tim/description4.png
[03:59] <thumper> https://devpad.canonical.com/~tim/description5.png
[03:59] <thumper> mwhudson: ^^
[03:59] <thumper> comments?
[04:00] <mwhudson> thumper: why a private server?
[04:00]  * mwhudson looks
[04:00] <mwhudson> bah
[04:00] <thumper> mwhudson: because it is easy?
[04:00]  * mwhudson wants focus follows eyes
[04:00] <mwhudson> fair enough
[04:00] <thumper> I could have copied them to penhey.net...
[04:00] <mwhudson> train your fingers to type people.canonical.com instead :-)
[04:00] <thumper> I have people.canonical.com?
[04:00] <mwhudson> yes
[04:00] <thumper> where?
[04:01] <mwhudson> tim@
[04:01] <thumper> or is it devpad?
[04:01] <mwhudson> same as everything else
[04:01] <mwhudson> no
[04:01] <mwhudson> it used to be rookery, but it's another machine now
[04:01] <mwhudson> lillypilly or something
[04:01] <thumper> heh, first time I've logged in to there
[04:03] <thumper> ok all on https://people.canonical.com/~tim/description*.png too
[04:03] <mwhudson> thumper: do you have a screeny of the description in the lower position when both description and commit message are set?
[04:03]  * thumper makes one
[04:05] <mwhudson> thumper: boring one for you
[04:05] <mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/bzr-2.1-gold/+merge/19678
[04:07] <thumper> http://people.canonical.com/~tim/description6.png
[04:07] <thumper> mwhudson: 6400 lines isn't trivial :)
[04:07] <thumper> mwhudson: proposed against db-devel?
[04:08] <mwhudson> thumper: crap
[04:08] <mwhudson> yeah
[04:08] <thumper> mwhudson: I want to be able to have an editor to change that
[04:08] <thumper> mwhudson: to save the whole delete, recreate screwage
[04:09] <mwhudson> thumper: yes please
[04:09] <mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/bzr-2.1-gold/+merge/19679 then
[04:11] <thumper> done
[04:11] <mwhudson> three things in ec2
[04:12] <mwhudson> might be enough for today...
=== adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[09:01] <adeuring> on call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[09:07] <henninge> adeuring: was that ^ meant to go into the subject? ;-)
[09:07] <adeuring> henninge: argh, yes...
[09:07]  * adeuring needs more coffee
=== adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[09:08] <henninge> adeuring: cool, that means you have time for my branch! ;-)
[09:08] <adeuring> henninge: sure, but first more coffee ;)
[09:08] <henninge> adeuring: whatever you need ... ;)
[09:08] <henninge> https://code.edge.launchpad.net/~henninge/launchpad/bug-523810-needs-information-age/+merge/19689
=== henninge changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [henninge] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[09:09] <henninge> adeuring: thanks
[10:24] <adeuring> henninge: review sent. I have a few questions
[10:25] <henninge> adeuring: thanks, I'll look at it
[10:29] <henninge> adeuring: about the placement of the constants in the inteface module.
[10:29] <adeuring> henninge: yes?
[10:30] <henninge> adeuring: I remember a rule that browser and test code should not import from model code.
[10:30] <adeuring> henninge: yes, you're right
[10:30] <henninge> adeuring: I have to admit, I have not found anybody with that same memory yet .... ;)
[10:30] <henninge> adeuring: oh, there you are! ;-D
[10:31] <henninge> adeuring: that's why I moved this into the interface module.
[10:31] <adeuring> ok, so let's leave it there
[10:31] <henninge> adeuring: also, I consider these expiration times as implementation-independent.
[10:31] <adeuring> ok
[10:31] <henninge> and that is the idea of the interface-model split, isn't?
[10:32] <adeuring> henninge: in general, yes. But it is a bit ober-bureaucratic in ths case, IHMO ;)
[10:33] <adeuring> s/ober/over/
[10:33] <henninge> ;)
[10:34] <henninge> adeuring: but I don't see a pressing reason not to have this in the interface module
[10:34] <adeuring> right
=== henninge changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[11:05] <henninge> adeuring: replied ;)
[11:05]  * adeuring is looking
[11:09] <adeuring> henninge: r=me. Thanks for the changes!
[11:09] <gmb> adeuring: Do you have time to review https://code.edge.launchpad.net/~gmb/launchpad/link-attachment-bug-524296/+merge/19694 ?
[11:11] <henninge> adeuring: thank you
=== gmb changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [gmb] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== matsubara-afk is now known as matsubara
[11:32] <adeuring> gmb: r=me, one small additional request
[11:32] <gmb> adeuring: Thanks.
=== adeuring changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews abgeändert
[11:33] <adeuring> gmb: welcome :)
=== salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews abgeändert
=== salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== noodles775 changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: - || queue [noodles] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[12:09] <noodles775> Hi guys, one for the queue: https://code.launchpad.net/~michael.nelson/launchpad/ppa-privatisation-test-refactor2/+merge/19696
[12:10] <noodles775> ... if you don't mind me being gone for lunch :)
[12:11] <adeuring> noodles775: I'll have lunch too soon, but after that i can take it.
=== mrevell is now known as mrevell-lunch
[12:30] <bac> hi adeuring, salgado: can i add https://code.launchpad.net/~bac/launchpad/bug-487793/+merge/19700 to the queue?
=== bac changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: - || queue [noodles,bac] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[12:31] <salgado> bac, sure; I'll take it
[12:31] <bac> thx
=== salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,noodles || queue [bac] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -,bac || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[13:22] <noodles775> Thanks for the review salgado
[13:24] <noodles775> Sheesh, I've no idea what the garbage in the testing factory...
[13:24] <adeuring> salgado: can you please review this MP: https://code.edge.launchpad.net/~adeuring/launchpad/bug-201015-bug-branch-search/+merge/19699?
[13:25] <salgado> adeuring, sure.  would you like to take bac's so that I go straight to yours?  otherwise I'll do yours after I finish his
[13:25] <adeuring> salgado: sure, np
=== adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: bac, adeuring || queue [-] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== mrevell-lunch is now known as mrevell
[13:28] <noodles775> salgado: so the stuff that looks like garbage in factory.py is actually in trunk, it's a sample diff used by some factory methods. My editor automatically removed some trailing whitespace from it.
[13:29] <salgado> oh, I see.  the lines starting with a white space followed by a '+' are just context to your diff
[13:29] <noodles775> Yep.
[13:30] <salgado> cool
[13:30] <jtv> salgado, one more for the queue: https://code.launchpad.net/~jtv/launchpad/bug-523926/+merge/19704
=== salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: bac, adeuring || queue [jtv] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[13:30] <jtv> thanks
[13:45] <salgado> adeuring, did you ask for a UI review of your branch?  a UI reviewer might have other ideas for where to place the new radio button
[13:45] <adeuring> salgado: no, not yet...
[13:49] <salgado> adeuring, is there a compelling use case for including only the bugs *without* a linked branch?  I don't see a similar option for including only bugs without patches/CVEs
[13:50] <salgado> disclaimer: I haven't read the bug report yet
=== sinzui changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: bac, adeuring || queue [jtv, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[13:50] <adeuring> salgado: well, the wish is mentioned in the bug report ;)
[13:50] <adeuring> salgado: and a use case is: "look for bugs that seem to be uninished", for example
[13:53] <salgado> adeuring, right, but having support for that makes the radio button necessary, thus adding some complexity and making for a not-very-clear UI
[13:53] <adeuring> right
[13:55] <salgado> I'd rather address just the most common use case (searching only for bugs with linked branches), which would allow for a simpler UI.  and later, if necessary, we can extend it
[13:56] <salgado> but if we can come up with a simple UI that accommodates both use cases, I'd be ok, I think
[13:56] <salgado> although we might be adding a feature that may never be used
[13:56] <salgado> adeuring, did you have a pre-implementation call?
[13:57] <adeuring> salgado: no...
[13:57] <adeuring> salgado: shall we also ask the ui reviewer about the "no branches" option?
[13:58] <salgado> adeuring, sure, but it might be a good idea to first decide whether or not it's worth allowing users to search for bugs without branches
[14:00] <adeuring> salgado: well, one option is to search fro bugs which are "unfinished", even if this simply means that a branch in fact exists but is not linked.
[14:00] <adeuring> We have the use case to better suport upstream
[14:00] <adeuring> so being able to provide upstream with branches for bugs seems to be useful
[14:01] <salgado> adeuring, can you bring that up for discussion with deryck?  I think you guys are better equipped than myself to decide whether or not it's useful
[14:01] <adeuring> ok
[14:02] <salgado> I was just trying to ensure we discussed it before going any further, to avoid having to rework later or trying to simplify a UI that's may not be necessary
[14:03]  * deryck looks at the initial bug report
[14:08] <deryck> adeuring, salgado -- I haven't looked at the UI for this, i.e. haven't branched and run this myself, but if the UI is horrible to enable the negative, I would lean toward just searching for bugs *with* branches.
[14:09] <adeuring> deryck: let me make a sreecnshot...
[14:09] <deryck> adeuring, salgado -- hearing you guys discuss this, though, I do wonder why a radio button is required.  maybe a "with branch" and "without branch" check box, and just form validate that only one is selected.  Or else do nothing different than the default. :-)
[14:10] <deryck> adeuring, ok, will wait on screenie
[14:10] <adeuring> deryck: an interesting idea ;)
[14:11] <salgado> deryck, that wouldn't be a great UI either, I think
[14:11] <adeuring> deryck, salgado: http://people.canonical.com/~adeuring/branchsearch.png
[14:16] <deryck> adeuring, salgado -- so I don't think the radio is bad.  I think it should be made to look like the affects option -- a check for "show with branches" (or some better phrase) and then "with linked branch" and "without linked branch" toggle.
[14:17] <deryck> we shouldn't have to have it toggled on for the default.  but admittedly, I'm thinking only UI here and not what's involved to code this.
[14:17] <salgado> ain't that how we should think when designing the UI? ;)
[14:18] <adeuring> deryck: nice idea!
[14:18] <deryck> heh
[14:18] <deryck> salgado, adeuring -- of course, this is just an idea.  I'm not a qualified UI reviewer. :-)  I would pre-imp with a ui person.
[14:19] <adeuring> deryck: but... I think I know what you mean, but the "any" and "all" buttons below "show bugs affecting me" relalte to the "tags" field on left ;)
[14:19] <salgado> yeah, it took me ages to realize that
[14:21] <deryck> yeah, so it's not a perfect idea.  and the grouping of the options could generally be better.
[14:22] <salgado> adeuring, care to check with a UI reviewer what they think of these two options?
[14:22] <adeuring> noodles775, sinzui: ^^^^
[14:24] <sinzui> adeuring: noodles775: I can do it in 30 minutes
[14:25] <noodles775> OK, thanks sinzui.
=== BjornT changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: bac, adeuring || queue [jtv, sinzui, bjornt] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[14:36] <BjornT> adeuring, salgado: here's a small patch for you to review when you have time: https://code.edge.launchpad.net/~bjornt/launchpad/quiet-update-sourcecode/+merge/19710
[14:37] <adeuring> I'll look at it
[14:37] <BjornT> thanks
=== salgado is now known as salgado-lunch
=== sinzui changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: bac, adeuring || queue [jtv, sinzui, bjornt, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[15:04] <adeuring> BjornT: r=me
=== adeuring changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: jtv, adeuring || queue [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[15:08] <BjornT> thanks adeuring
=== matsubara is now known as matsubara-lunch
=== salgado-lunch is now known as salgado
=== bigjools changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: jtv, adeuring || queue [sinzui, sinzui, bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[16:09] <jtv> rockstar: can you check if I understood you correctly?  https://code.edge.launchpad.net/~jtv/launchpad/bug-523449/+merge/19622
[16:10] <jtv> rockstar: and its sibling, https://code.edge.launchpad.net/~jtv/launchpad/bug-507681/+merge/19531
[16:10] <sinzui> adeuring: I am sorry my review is taking so long. bugs advanced search has many ui bugs that have irked me. Instead of doing a simple review, I am looking at why this page is different from other forms and will propose a fix instead of asking you to find the fix
[16:10] <adeuring> sure, sounds good!
[16:11] <rockstar> jtv, so basically, you kept moved some work into the second branch that could have been in the first?
[16:12] <jtv> rockstar: yes
[16:12] <jtv> rockstar: I moved the "if work_is_appropriate: work()" structure into the utility.
[16:13] <jtv> Could've done better, I know.
[16:13] <rockstar> jtv, it's fine, was just a bit confusing.
=== salgado changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: jtv, adeuring || queue [bigjools] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== matsubara-lunch is now known as matsubara
[16:24] <salgado> bigjools, around?
=== salgado changed the topic of #launchpad-reviews to:  on call: adeuring,salgado || reviewing: jtv, bigjools || queue [adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[16:25] <salgado> adeuring, I'll get back to yours once we've heard from sinzui, ok?
[16:25] <adeuring> salgado: sur
=== adeuring changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -, bigjools || queue [adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[16:27] <sinzui> adeuring: salgado: I am almost done. I have discovered that advanced search is not just a handcrafted form, all the markup and css rules for legend were updated for all forms except this one. I think I can propose a fix
[16:28] <adeuring> sounds good
[16:28] <salgado> cool; that's great
[16:34] <bigjools> salgado: yes
[16:35] <salgado> bigjools, I was just wondering if it'd be possible to use a newly created distribution in that test, to have it not depending on any sample data
[16:36] <bigjools> salgado: that would be quite a lot of work :/
[16:37] <salgado> I imagined
[16:37] <bigjools> the other tests in the file all do the same thing
[16:38] <salgado> bigjools, do we need to have the suffixes hard-coded there?  can't we take them from a constant somewhere?
[16:38] <bigjools> salgado: that's the only place they will appear - is there a benefit to doing that?
[16:39] <salgado> in that case, no
[16:39] <salgado> approved. :)
[16:39] <bigjools> one of these days we'll get rid of the sampledata crack for Soyuz
=== salgado changed the topic of #launchpad-reviews to: on call: adeuring,salgado || reviewing: -, - || queue [adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
[16:40] <bigjools> salgado: cheers
[16:40] <bigjools> salgado: I'd love to replace this particular sample data with a function that will set it up instead
[16:41] <bigjools> check out lucilleconfig if you want to see why that's tough :)
[16:42] <salgado> I believe you it's tough, and that's why I'm not convinced we'll ever manage to get rid of sampledata for soyuz/registry/bugs tests
[16:43] <bigjools> well tough doesn't mean impossible :)
[16:43] <bigjools> it will just take a very long time :/
[17:13] <jtv> rockstar: any chance of finishing the review today?
[17:13] <rockstar> jtv, which review?
[17:13] <jtv> rockstar: oh, I since you commented, I thought you were reviewing my branch.
[17:13] <jtv> well, and since you promised to look at the changes.  :-)
[17:14] <rockstar> jtv, haven't I approved everything outstanding?
[17:14]  * rockstar looks
[17:16] <jtv> rockstar: ah, so you did!  Thanks.  Whole lotta landing coming up...
[17:51] <rockstar> abentley, https://code.edge.launchpad.net/~rockstar/launchpad/bug-517266/+merge/19730
=== deryck is now known as deryck[lunch]
=== danilos is now known as daniloff
[18:47]  * maxb would like to enqueue https://code.edge.launchpad.net/~maxb/launchpad/bug-497731/+merge/19732
[18:52] <salgado> maxb, I'll take it
[18:53] <maxb> thanks
=== deryck[lunch] is now known as deryck
[19:03] <salgado> maxb, is get_bzr_plugins_path() still used anywhere?
[19:03] <maxb> Nowhere outside that file. I was wondering whether to remove it from __all__
[19:05] <salgado> yeah, that's why I asked.  maybe we should just change it instead of having a separate one?
[19:06] <salgado> after all, the only behaviour we want is the one of get_BZR_PLUGIN_PATH_for_subprocess(), right?
[19:07] <maxb> It wouldn't even suck to just inline get_bzr_plugins_path at its two call-sites in lp/codehosting/__init__.py
[19:08] <maxb> I agree I can't imagine why any code other than the load_plugins([...]) call itself should want the directory alone
[19:09] <salgado> I'd be against inlining, but I'm all for keeping just one version of it -- the one that does the right thing. :)
[19:11] <maxb> So your suggestion is to just drop it from __all__ ?
[19:12] <salgado> no, just remove get_BZR_PLUGIN_PATH_for_subprocess() and change get_bzr_plugins_path() to append a "-site" to its return value
[19:16] <maxb> That's not acceptable because the bare directory is required for the load_plugins([...]) call ~10 lines below
[19:17] <salgado> oh, I missed that
[19:18] <salgado> why is it required there?
[19:19] <maxb> The way it works is that if you're calling that bzrlib call directly, bzrlib uses *exactly* the directories you pass it. If you're setting the environment variable, bzrlib augments it with the standard directories unless you use the magic tokens to tell it not to
[19:20] <salgado> I see
[19:20] <sinzui> bac: I approved the UI, but you need to fix the links first.
[19:21] <bac> sinzui: i'm just reading it now
[19:22] <sinzui> EdwinGrubbs: I just used the link to upstream multi-step form. It is very nice. It feels faster than the series picker. I think that has a lot to do with the design shown only the information you need to know
[19:22] <salgado> maxb, then we can rename the existing one to _get_bzr_plugins_path() and remove it from __all__
[19:22] <maxb> Sounds good
[19:23] <salgado> maxb, also, it'd be nice to state in the new docstring why we use the "-site" magic token
[19:23] <EdwinGrubbs> cool
[19:24] <salgado> 65	             args, env_changes={'BZR_SSH': 'paramiko',
[19:24] <salgado> 66	-                               'BZR_PLUGIN_PATH': get_bzr_plugins_path()},
[19:24] <salgado> 67	+                'BZR_PLUGIN_PATH': get_BZR_PLUGIN_PATH_for_subprocess()},
[19:24] <salgado> maxb, can you also align the two dict keys in there too?
[19:25] <maxb> pull the 'BZR_SSH' down onto a new line?
[19:25] <salgado> yeah, that's what I'd do
[19:32] <maxb> "The '-site' token tells bzrlib not to include the 'site specific plugins directory' (which is usually something like /usr/lib/pythonX.Y/dist-packages/bzrlib/plugins/) in the plugin search path, which would be inappropriate for Launchpad, which may be using a bzr egg of an incompatible version." ?
[19:34] <salgado> sounds good to me
[19:34] <maxb>         return self.run_bzr_subprocess(args,
[19:34] <maxb>             env_changes={
[19:34] <maxb>                 'BZR_SSH': 'paramiko',
[19:34] <maxb>                 'BZR_PLUGIN_PATH': get_BZR_PLUGIN_PATH_for_subprocess()
[19:34] <maxb>             },
[19:34] <maxb>             allow_plugins=True, retcode=retcode)
[19:34] <maxb> Is that formatting acceptable?
[19:34] <salgado> it's not -- the two arguments to run_bzr_subprocess() should be aligned
[19:36] <maxb>         return self.run_bzr_subprocess(
[19:36] <maxb>             args, env_changes={
[19:36] <maxb> ?
[19:36] <salgado> that works, yes
[19:36] <salgado> args, env_changes={
[19:36] <salgado>     'BZR_SSH': 'paramiko',
[19:37] <salgado>     'BZR_PLUGIN_PATH': get_BZR_PLUGIN_PATH_for_subprocess()
[19:39] <maxb> ok, pushed
=== salgado is now known as salgado-afk
[19:46] <EdwinGrubbs> sinzui: I didn't see an email from you on the mockups. Do you want to discuss that today or wait till Monday?
[19:47] <sinzui> EdwinGrubbs: sorry. I am still doing reviews. I do need to send an email because I need to collect my thoughts
=== EdwinGrubbs is now known as Edwin-lunch
[19:52] <bac> sinzui: thanks for catching the link issue.  here is a screenshot of the revision.
[19:52] <bac> http://people.canonical.com/~bac/link-portlet-fixed.png
[19:53] <sinzui> bac: That is lovely
[19:54] <bac> i think that's a bit of a stretch but i'll take it and run off to ec2
=== henninge is now known as henninge-afk
=== salgado-afk changed the topic of #launchpad-reviews to: on call: adeuring || reviewing: -, - || queue [adeuring] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews
=== matsubara is now known as matsubara-afk
[21:24] <sinzui> bac: Edwin-lunch: I have spent 7.5 hours doing just UI reviews and proposals. I am burned out.
[21:24] <bac> sinzui: and you should be burned out
=== Edwin-lunch is now known as EdwinGrubbs
[21:35] <EdwinGrubbs> sinzui: well, I hope you enjoy your weekend.
[21:36] <sinzui> I wont. I am told I am attending multi-cultural night at my daughter's school and there will not be any food from asian :(
[21:37] <sinzui> My youngest daughter caught my wife playing tooth faerie. I need to hack her computer to destroy the evidence
[21:37] <sinzui> This same daughter just dressed the kittens in doll clothes
[21:38] <sinzui> My son who  must has Asbergers just found all the puzzles I hid and is assembling them in my bedroom
[22:10] <bac> sinzui: smuggle in some sriracha and put it on everything
[22:11] <sinzui> well, I think that is an excellent idea. I will report back
[22:11] <rockstar> Can anyone do a really simple review for me?
[22:15] <rockstar> bac? ^^
[22:15] <bac> rockstar: yes.  i'm a very simple reviewer.
[22:16] <rockstar> bac, creating mp now.
[22:26] <rockstar> bac, https://code.edge.launchpad.net/~rockstar/launchpad/js-no-bigger-than-512k/+merge/19751
[22:30]  * bac looking
[22:33] <bac> hi rockstar
[22:33] <rockstar> bac, hi.
[22:33] <bac> it looks good but i have three comments
[22:33] <rockstar> bac, shoot.
[22:33] <bac> 1 - add #!/usr/bin/python
[22:33] <rockstar> bac, ah, yes.
[22:34] <rockstar> Shouldn't it be python2.5 ?
[22:34] <bac> 2 - dude, it's mid-february, start using 2010 already
[22:34] <rockstar> Ugh, c-n-p error.
[22:34] <bac> 3 - should max size be 512 * 1024 ?
[22:35] <rockstar> bac, it could be, yeah.  I didn't think it was that big of a deal, but I'll changed it.
[22:35] <bac> ok, cool
[22:35] <bac> i'll add those to the MP and approve it
[22:35] <rockstar> bac, fixes pushed.
[22:37] <bac> done