Re: PATCHES: more refactoring and skeletal cvs2hg

On Sat, Aug 29, 2009 at 4:59 PM, Michael Haggerty<[email protected]> wrote:
>> 2) GitRunOptions, SVNRunOptions: use super() where appropriate.
>> http://vc.gerg.ca/hg/cvs2svn-patches/file/tip/use-super.patch
>
> What exactly is the advantage of using super() in a trivial inheritance
> tree (i.e., in the absence of multiple inheritance)?  It seems to me
> that it just adds verbosity without making the code less error-prone.
> No doubt I have overlooked something...

IMHO it *does* make the code less error-prone: you can modify the
class hierarchy without touching all the super() calls.  E.g. with
old-style superclass calls, changing from

  A -> B       ("A is superclass of B")

to

  A -> A1 -> B

means that you also have to update all the A.xxx(self, ...) calls in
B.  With super(), no such change is necessary.  I've always assumed
that was the main benefit of super().  Unfortunately, it still doesn't
protect you from renaming B or moving methods between classes.  (To
paraphrase Larry Wall, "Java has to be better at something". ;-)

Regardless: let me know if you want me to drop the patch.  No feelings
hurt either way.

>> 3) Reduce callback_manpage() duplication between SVNRunOptions and
>> GitRunOptions.
>> http://vc.gerg.ca/hg/cvs2svn-patches/file/tip/refactor-callback-manpage.patch
>
> The long function calls could be shortened by making short_desc,
> synopsis, long_desc, etc members of the particular RunOption class, no?

Good idea.  Another idea is to introduce a ManPage class where each
*RunOption subclass defines a ManPage instance, but that's probably
overkill.  I'll play around with your idea and see if it works.

>> 5) Add cvs2hg top-level script.
>> http://vc.gerg.ca/hg/cvs2svn-patches/file/tip/cvs2hg-main.patch
>
> This is also not something that was introduced by you, but it seems to
> me that it would make more sense to move the main() function to file
> run_options.py (maybe even make it a method of RunOptions?), and to move
> the foo_main() options to foo_run_options.py (maybe even make them a
> method of FooRunOptions?)

Hmmm.  IMHO the duplication between cvs2{svn,git,hg,bzr} is worse than
the duplication in main.py.  (And arguably I've made it worse by
introducing the Mercurial API version check in cvs2hg, thereby making
it so the scripts are not trivial copies of each other. Feel free to
call me on *that*.  ;-)  I'd sooner spend time doing something about
that duplication, honestly.

>> 7) run-tests: add test of cvs2hg man page generation
>> http://vc.gerg.ca/hg/cvs2svn-patches/file/tip/run-tests-cvs2hg-man.patch
>
> This makes run-tests.py fail for me, because I don't have Mercurial
> version 1.1 or later installed.  This is not OK, because Mercurial is
> not a prerequisite for cvs2svn.

Oops!  Will fix.

Greg

------------------------------------------------------
http://cvs2svn.tigris.org/ds/viewMessage.do?dsForumId=1667&dsMessageId=2388703

To unsubscribe from this discussion, e-mail: [[email protected]].