Merge lp:~lifeless/launchpad/uniqueconfig into lp:launchpad

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.

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.

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

Henning

On Tue, Oct 19, 2010 at 5:42 AM, Henning Eggers
<email address hidden> 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
conten...

On Tue, Oct 19, 2010 at 8:15 AM, Robert Collins
<email address hidden> wrote:
> Thanks for the review.

....

Wow, that was a pretty intense reply I sent - out of proportion to
your particular review.

 I want to be clear that I'm really addressing a generic systemic
issue I've been perceiving and thinking (and discussing around and
about) - your review was simply caught me first thing in the morning
and I guess the back of my head had finally come up with some clear
statements about the issues I see.

We probably want to take this to the list :)

-Rob

On Oct 19, 2010, at 02:15 , Robert Collins wrote:

> On Tue, Oct 19, 2010 at 5:42 AM, Henning Eggers
>
>
>> 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?

The review team is, of course, a democracy. I think Henning was probably just looking for a sanity check before involving the entire team.

As Rob indicated, the last few AsiaPac reviewers meetings have spent considerable time talking about lots of wide-ranging topics regarding reviews. Some of those ideas have been written up now in a subsequent email Rob sent proposing the optional review experiment.

A discussion of eliminating tech debt was brought up by Curtis recently in the AmEu meeting ("don't add to canonical.launchpad as we're trying to get rid of it") and was uncontroversial. We all agree tech debt is a burden for many reasons and in a perfect world we would make it go away. Curtis proposed one approach and most people thought it was reasonable. Robert has a different view, as expressed here, and in the AsiaPac meetings.

The topic comes up in the reviewers meeting because a) it is the only meeting where we all get together regularly, ignoring the fact we are forced to do it regionally with a single conduit (me), and b) the review is the last chance for a peer to catch the fact you may be contributing to tech debt or simply not cleaning it up.

The "you touch it, you fix it" mode of working is certainly not fair but it does cut across the board if we all agree to the idea. I have no qualms as a reviewer saying "I know you didn't create this problem but while you're there would you spend a few minutes to clean it up?" Most people are happy to oblige, not because I am wielding enormous reviewer power, but because we all agree we want to get rid of warts.

As to the fear that people will be unwilling to wander into the dark corners of the code base for fear they will be accountable for cleaning it all up, I'm not convinced that will be the way most of us will work. Our enforcement of cleaning up things you touch is not heavy handed. Henning asked that you have a follow-on branch but did not demand it as a condition of approval. It is simply our agreement that we'll all share, more or less, in the less interesting chore of cleaning up after ourselves.

Robert I think your views expressed here are bigger than the issue at hand and represent an idea that our development process, not just our review process, is fundamentally flawed. I think you need to find the appropriate forum to make suggestions and solicit input from everyone.

On Tue, Oct 19, 2010 at 11:32 PM, Brad Crittenden
<email address hidden> wrote:

> As to the fear that people will be unwilling to wander into the dark corners of the code base for fear they will be accountable for cleaning it all up, I'm not convinced that will be the way most of us will work.  Our enforcement of cleaning up things you touch is not heavy handed.  Henning asked that you have a follow-on branch but did not demand it as a condition of approval.  It is simply our agreement that we'll all share, more or less, in the less interesting chore of cleaning up after ourselves.

This isn't a fear I'm expressing, it's direct and indirect
observation; just today I had an anecdote repeated to me of someone
being told (by their mentor in the job when they joined) 'dont touch
that thing, you'll be asked to clean it up'.

> Robert I think your views expressed here are bigger than the issue at hand and represent an idea that our development process, not just our review process, is fundamentally flawed.  I think you need to find the appropriate forum to make suggestions and solicit input from everyone.

Oh, I'm sure they are, and those things are being discussed.

I didn't think I was being required to do a follow on branch; but
henninge expressed deep concern about an interest payment on tech
debt; thats evidence that something is wrong: we have the tech debt
already, its a chilling effect to be concerned about working on things
which have tech debt *because they have debt*.

I have two things that are important here:
 - what to do with this patch
 - moving the larger discussion forward

The second aspect is already underway.

-Rob

Hi Robert,
let me clarify in reverse order. ;-)

I picked Brad because he is kind of the head reviewer (by experience, otherwise we are democratic as mentioned) and because he currently works nearer to your time zone than I do. I was unavailable this (my) morning but it is always good to get a second opinion.

I guess I am too Launchpad-centric to notice http://pypi.python.org/pypi/fixtures. I looked for fixtures on Launchpad and found https://launchpad.net/python-fixtures which now seems out of date to me. A link to python.org would be a real improvement here. I have never been confronted with assessing external dependencies and am surprised to hear we don't audit them currently. That sounds like a hole in our process.

As Brad already mentioned, we do follow the strategy that whoever touches dirty code should fix it. I find that a good strategy because otherwise nobody will ever take the time to pay back tech debt. Instead we are just paying interest on top of it, as you correctly put it. Adding new files to the old structure is a lot of interest. I think the best approach in this case would be to create your new files at the new place (lp.services.config sounds good) and file a bug about moving the remaining canonical.config code there, too. Ideally you'd assign that bug to you and go about fixing it next.

Cheers,
Henning

Hi Henning, thanks for clarifying.

We can discuss the tech debt strategy separately; my views don't need
repeating right now. The reason I really don't want to put the new
file in a different place is that that *increases* the amount of added
tech debt: rather than an interest payment, its interest + a
withdrawal.

-Rob

Thanks for the XXX's and the bug. ;-) The other changes look good, too. Please remember to add the "config.is_testing" property soon to get rid of the multiline if statements.