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

On Thu, November 25, 2010 4:46 am, Vincent Ladeuil wrote:
> - quoting arguments: every single argument may contain spaces so
> internally using a splitted commandline and working on each
> should reduce the need to handle the quoting yourself (there
> are probably cases where you want to handle the quotes in the
> command line itself but I see no reason to not reuse the
> feature available in bzrlib.cmdline). All in all, I feel that
> if you consider the value of the config option as a command
> line and rely on bzrlib.cmdline to split it correctly, you
> won't have to handle so many special cases in *your* code ans
> implfiy it accordingly. This means no more *quote(arg* or
> *arg_quote* functions in bzrlib.mergetools,

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.

> - argument interpolation, using {b} instead of %b. I'd mentioned
> using longer option names too :) But as far the reference
> syntax is concerned, I think more people agree on {} rather
> than % (it's ok to have your own implementation to start with
> as you've done so far, implementing in config is my next target
> in this area (but there are other areas I'm working on too :-/)),

Agreed. I'll take care of this. I'll use full word tokens like {this}. 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.

> - 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.

> - why not making the tool name mandatory ? This will greatly
> simplify many parts of the code. I don't think mergetools are
> defined so often that we need to support guessing a name there
> especially when this name will be specified explicitely in all
> other cases,

Good point. I like my sugar but this can be removed.

> - while you took care to handle errors, there are no tests to
> cover them. Please add them, test coverage for errors is
> crucial, especially when dealing with user interactions,

Ok.

> - many functions accept a config parameter but default to the
> global config object. While this may makes sense for testing
> purposes, I'm affraid it will tend to defeat the way we want to
> use configs (ideally the *user* can define the merge tools in
> the most convenient way and we will find it in any config
> file), so the config parameter should be mandatory. In turn,

Ok.

> this means you're adding features to the config object itself,
> so until we provide better ways to do that, you should use the
> actual pattern which is to define specialized accessors there
> (see get_mail_client, get_change_editor,
> get_user_option_as_list etc). This probaly means no more
> {set|get}_merge_ functions in bzrlib.mergetools.

I dislike the monolithic-config-which-knows-all design of bzrlib.config.
It seems to me that it would be better to keep bzrlib.config focused on
providing generic access to the config files with reusable functions for
setting and getting various types and structures of data in the config,
while config code specific to particular parts of bzr are actually located
with the code that uses it. Otherwise, code related to a particular
component, such as mergetools, is scattered across the codebase.

> - 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?

> Moving the accessorts in bzrlib.config may be the most important
> one in my eye. There is already too many duplication in existing
> plugins that were forced to enhance the config features by
> re-implementing specialized config objects. So let's try to avoid
> the duplication of effort here. This will also means that we can
> focus on bzrlib.config when we enhance it in the future.

I think plugins and internal clients of bzrlib.config would be better
served by generic reusable improvements to the config objects. Adding
set_this_modules_value and set_other_modules_value and set_kitchen_sink to
bzrlib.config seems like bad design to me.

>> + def is_available(self):
>> + executable = self.get_executable()
>> + return os.path.exists(executable) or
>> _find_executable(executable)
>>
>
> Why not make the os.path.exists() call part of _find_executable() ?

One function to do one thing. I should rename it to
_find_executable_on_path to be more descriptive though.