launchpad-reviewers team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

On Tue, Oct 19, 2010 at 5:42 AM, Henning Eggers
<henning.eggers@xxxxxxxxxxxxx> wrote:
> Hi Robert,
> as mentioned on IRC, I don't feel good about adding new files to the canonical tree if they are Launchpad specific. The feeling has not gone away and I ask you to consider moving canonical.config to lp.config. This will most likely require an extra branch as there are many call sites for canonical.config but they are all mechanical changes.

Thanks for the review.

I think that it is a separate concern - I appreciate and can share it,
but the debt of having lp specific stuff in canonical module *in the
lp source tree* already exists, I don't see the benefit to us of
blocking *any* improvement due to that existing problem. Happy to work
on that separately : unhappy - extremely unhappy - at tying orthogonal
(related but different concerns) together.

Reviews *should* be catching functional and maintenance issues *with
changes presented*, not assessing the global state of the system.
Assessing the global state would be a time consuming and depressing
task - and every review would have a huge pile of tech debt land on
it, which is unreasonable to the submitter, to the reviewer, slows
cycle time down and creates a disincentive to touch areas of the code
base associated with tech debt (because the toucher will be asked to
do more as a condition of landing their code). This is unreasonable.

Its very reasonable to say 'hey, what you are doing contains a tech
debt interest payment. It would be wonderful if you could follow up
with removal of that tech debt before you consider yourself finished
with whatever arc of changes you're working on'.  In this case I'm
adding a module to a package. The package is mislabelled (nowadays) as
being canonical specific when its lp specific. Moving the entire
module at once isn't going to be easer or harder due to having a new
submodule [well ok, a /tiny/ bit harder]. In particular, if the
original tech debt was paid off, we wouldn't be paying interest;
putting the new code elsewhere in the tree would split things that
shouldn't be; having the existing tech debt get bigger is paying
interest on it. So sure, I can move the config module to
lp.services.config if I get some time, and I (obviously) would do that
as a separate branch. But I see many reviews saying (paraphrased) 'you
touched something dirty, you need to clean your hands and the dirty
thing'. This really does create an incentive not to touch dirty
things.

Thats very different to doing something that adds brand new tech debt
(which actually, and slightly embarassingly I did very recently, but
I'm definitely going to get to that one :))

> Also, I am worried about your bumping the version number for fixtures to 0.3.2 when that version is not available from the project page or branch on Launchpad. Neither was 0.3.1 it seems. Intentionally or not, this sneeks unreviewed and unproven code into the Launchpad tree. I have a bad feeling about this practice, too.

http://pypi.python.org/pypi/fixtures has 0.3.2 and had that before
this branch was pushed. Its true it wasn't in trunk, that was
oversight. As for changing dependencies, we should either review [the
content of changes to] external deps consistently - perhaps with a
risk assessment, or we shouldn't.

Currently we don't : consider python-dns using *secure crypto* at
startup (blocks for 100's of seconds running parallel tests) (which if
the appservers use it will cripple fast restarts when we do one
appserver per worker thread). bzr, buildout, grok, manuel, mechanize,
paramiko, Paste, pydkim, zope etc, etc, etc - they all have varying
sized communities, some with code review, some without. I don't know
why you think using a focused library I happen to maintain is sneeking
code into Launchpad, but its certainly not : the code that is in
Launchpad is a client of it, is able to be tested and critiqued, and
if the APi of the external library is a problem, we can, in Launchpad,
either choose to adapt it, fix it upstream, or stop using it.

I think we should embrace having less unLaunchpad code in the
Launchpad tree - Launchpad is, by my estimate, 25-35 percent
duplication/waste/unrelated things piled in.

> Please talk to Brad about both issues and see what he says.

I'm not sure why Brad specifically - the review team was always a
democracy ever since it was formulated : we really want broad input,
still we can start with Brad... Brad, what do you think?

-Rob
-- 
https://code.launchpad.net/~lifeless/launchpad/uniqueconfig/+merge/38689
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~lifeless/launchpad/uniqueconfig into lp:launchpad/devel.