On Tue, Oct 19, 2010 at 5:42 AM, Henning Eggers <[email protected]> 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. _______________________________________________ Mailing list: https://launchpad.net/~launchpad-reviewers Post to : [email protected] Unsubscribe : https://launchpad.net/~launchpad-reviewers More help : https://help.launchpad.net/ListHelp
