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

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

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

Cool.

    > 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

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

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

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 :(

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

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

See above.

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

Right, that doesn't forbid changing functions that are introduced to
better fit their use cases :) Especially when they are used only once...

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 ?