Code review comment for lp:~doxxx/bzr/mergetools

On Thu, November 25, 2010 11:11 am, Vincent Ladeuil wrote:
>>>>>> Gordon Tyler <email address hidden> writes:
>
> <snip/>
>
> > The MergeTool class *does* use a splitted commandline
> > internally. The only time it has to use _quote_args is to preserve
> > spaces in arguments when converting it into single string form for
> > storage in the config.
>
> Argh, and we don't have reusable code for this use case ?
>
> Err wait, why do we *need* to care here ? When do you need to go from a
> list form to a line form ? All the user should have to deal with is the
> line form where quoting should be respected (or bugs fixed if
> bzrlib.cmdline is incorrect).

The user only has to deal with line form in the config. The quoting is
just for converting from split form in the MergeTool object into line form
for storing in the config.

Maybe what I should do is move this into the bzrlib.cmdline module to
provide a function which reverses the result of its split function?

> > I'm also thinking of adding tokens like {this.name}, which is the
> > basename of the this file, for use with tools which can take
> > additional parameters to set window titles nicely.
>
> Can we postpone this ? :D

Fine. :P

> >> - more NEWS entries :) This is a user facing change and as such
> >> also needs to be mentioned in the what's new,
>
> > The bzrlib.mergetools module itself is not user facing. I added
> stuff to
> > the what's new in the mergetools-command merge proposal since that
> > modifies user-facing commands.
>
> Config file content is user facing.

I suppose so. Except defining merge tools in the config does nothing
because there's nothing to use it. Yet. But I guess that's splitting
hairs. I'll move that stuff from the mergetools-commands proposal into
this one.

> Yeah right, see my proposal, I don't like to define specific accessors
> either, but that's what we have today. So to minimize the overall work,
> it's better to either:
> - fix the "wrong" API and then use it,
> - use the "wrong" API and then fix the API and all its uses
>
> This often means making a different proposal for the API change
> too. While this may seem heavy weight this is also the guarantee that
> people will use it instead of introducing their own.
>
> But starting new APIs here and there is worse as it means we never get
> to the new, better, APIs and instead spend time re-implementing the
> missing bits and re-fixing the same bugs :(

I see what you're trying to achieve. Okay, I'll move the config stuff into
bzrlib.config itself.

> >> - the %B related stuff is unclear (I wonder if it really need to
> >> addressed here instead of letting the caller handle it),
>
> > What %B stuff?
>
> Meh, %T. I mixed up TEMP and BASE as Gary mentioned the base file and %B
> in his comment.

It seems to be necessary for the meld tool. It's defined as 'meld %b %T %o'.

> >> Why not make the os.path.exists() call part of _find_executable() ?
>
> > One function to do one thing.
>
> Right, that doesn't forbid changing functions that are introduced to
> better fit their use cases :) Especially when they are used only once...

Point, I should just inline _find_executable. I kept it separate since I
copied it from a public domain source, but I suppose I can modify it how I
like.

> By the way, I'm a bit surprised we don't have the same need in other
> parts of the code base... let see...
>
> What are the differences here between your implementation and
> bzrlib.tests.ExecutableFeature._probe() ?
>
> Should the two implementations be unified ? If not why ?

Answer hazy, ask again later. ;)

I don't have access to bzr source code at the moment, I'll check later
tonight when I get home.