Re: PATCHES: more refactoring and skeletal cvs2hg

Greg Ward wrote:
> Finally, a series of patches that culminates in the addition of cvs2hg
> itself.  It doesn't *do* anything yet apart from parse options and
> generate a man page.  But it's a start.

Looks good, except for a couple of comments below.

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

> 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?
 Well, OK, it is ugly to define multiline strings within a class
definition.  But maybe it would be less ugly to write "short_desc =
_short_desc" or something within the class definition, or
"SVNRunOptions.short_desc = """..."""" *after* the main class definition?

(Of course this isn't a problem with your patch; I'm just thinking aloud.)

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

> 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.  Please make this test SKIP if the
requisite version of Mercurial is not installed.  (You can do this, for
example, by raising svntest.Skip() as is done in other places in
run-tests.py.)

Please let me know what you have changed when you resubmit because
otherwise your patch series was fine.

Thanks,
Michael

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

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