[PATCH 1 of 4] hgmerge: add new hgmerge package under mercurial

[Arve Knudsen]

On Jan 8, 2008 9:23 PM, Matt Mackall <mpm at selenic.com> wrote:

>
> On Tue, 2008-01-08 at 19:28 +0100, Arve Knudsen wrote:
> > On Jan 8, 2008 6:49 PM, Matt Mackall <mpm at selenic.com> wrote:
> >         > +class _pluginopts(object):
> >         > +    ''' Plug-in options handler baseclass.
> >         > +    '''
> >         > +    def __init__(self, ui, sectname, name):
> >         > +        self.attrs = {}
> >         > +        self.__ui, self.__sect, self.__name = ui, sectname,
> >         name
> >         > +        self._set_int('priority', default=0)
> >         > +
> >         > +    def _set_string(self, prop, **kwds):
> >         > +        self._set_opt(prop, self.__ui.config, **kwds)
> >         > +
> >         > +    def _set_bool(self, prop, **kwds):
> >         > +        self._set_opt(prop, self.__ui.configbool, **kwds)
>
> >         Yes, looks like someone's a Java fan.
> >
> > I don't write Java. I don't have anything against it for that matter
> > either .. This is more typical generic programming, which I  _am_ a
> > fan of. My design typically turns out the way it does based on (unit)
> > testability.
>
> The Java comment comes from the "everything is a class" mindset that it
> seems to promote. There are about as many classes in this code as the
> rest of Mercurial combined. And most of these are paper-thin and making
> them classes rather than functions or native datatypes just adds
> baggage. The result is there's so much code here, it'd take me quite a
> while to actually figure out what it's doing and review it. (1100 lines
> here, compared to 700 in the actual merge code!)
>
> The above appears to be support for doing foo.bar config elements, yes?
> Here are a few other ways to approach that. First, we could integrate it
> into ui so everyone could use it. Or second, we could give each merge
> tool its own section and simply call


>  ui.config("merge-" + tool, "executable")
>
> Or third, we could just call
>
>  ui.config("merge", tool + ".executable") # not so painful, really
>

Some of the code I wrote wraps ui.config because it lacks in certain
respects yes.  But the plug-in options hierarchy is useful since different
implementations can be derived for different kinds of plug-ins. In
particular, if we are to let people write custom merge plug-ins they may
want to define their own options. Some of the point of wrapping
ui.configwith helper methods is also to make it dead simple for new
plug-ins to
process options.

I suggest we handle one problem at a time though. If ui.config is to be
improved, why not do that at a later stage.


> I suppose we'll want to be able to iterate through the list of tools,
> which can be gotten with a simple list comprehension (candidate for a
> helper function). In any case, we should need just a few new lines of
> code, not a hierarchy of new classes.
>
> > > +def _readfile(fname):
> > > +    f = file(fname, 'rb')
> > > +    try: data = f.read()
> > > +    finally: f.close()
> > > +    return data
>
> >         This function should be taken out and shot.
>
> > I don't see why, but what do I know.
>
> What's it accomplishing? The only thing it does is close a file slightly
> earlier than it would otherwise be closed in the case of an exception.
> If that matters, we're probably hiding a real bug somewhere else.


We've removed this function now, btw.

Arve
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://selenic.com/pipermail/mercurial-devel/attachments/20080110/c2699169/attachment.htm