Re: [RFCv3 2/4] Add Python support library for CVS remote helper

[David Aguilar <davvid@gmail.com>]

On Wed, Aug 12, 2009 at 02:13:49AM +0200, Johan Herland wrote:
> This patch introduces a Python package called "git_remote_cvs" containing
> the building blocks of the CVS remote helper. The CVS remote helper itself
> is NOT part of this patch.

Interesting...

> diff --git a/git_remote_cvs/changeset.py b/git_remote_cvs/changeset.py
> new file mode 100644
> index 0000000..27c4129
> --- /dev/null
> +++ b/git_remote_cvs/changeset.py
> @@ -0,0 +1,114 @@
> +#!/usr/bin/env python
> +
> +"""Functionality for collecting individual CVS revisions into "changesets"
> +
> +A changeset is a collection of CvsRev objects that belong together in the same
> +"commit". This is a somewhat artificial construct on top of CVS, which only
> +stores changes at the per-file level. Normally, CVS users create several CVS
> +revisions simultaneously by applying the "cvs commit" command to several files
> +with related changes. This module tries to reconstruct this notion of related
> +revisions.
> +"""
> +
> +from util import *


Importing * is frowned upon in Python.

It's much easier to see where things are coming from if you
'import util' and use the namespaced util.foo() way of accessing
the functions.

Furthermore, you're going to want to use absolute imports.
Anyone can create 'util.py' and blindly importing 'util' is
asking for trouble.

Instead use:
from git_remote_cvs import util


> +class Changeset (object):
> +	"""Encapsulate a single changeset/commit"""

I think it reads better as Changeset(object)
(drop the spaces before the parens).

That applies to the rest of this patch as well.


This also had me wondering about the following:
	git uses tabs for indentation

BUT, the python convention is to use 4-space indents ala PEP-8
http://www.python.org/dev/peps/pep-0008/


It might be appealing to when-in-Rome (Rome being Python) here
and do things the python way when we code in Python.

Consistency with pep8 is good if we expect to get python hackers
to contribute to git_remote_cvs.


> +
> +	__slots__ = ('revs', 'date', 'author', 'message')


__slots__ is pretty esoteric in Python-land.

But, if your justification is to minimize memory usage, then
yes, this is a good thing to do.


> +	def __init__ (self, date, author, message):
> +		self.revs    = {}      # dict: path -> CvsRev object
> +		self.date    = date    # CvsDate object
> +		self.author  = author
> +		self.message = message # Lines of commit message

pep8 and other parts of the git codebase recommend against
lining up the equals signs like that.  Ya, sorry for the nits
being that they're purely stylistic.

> +		if len(msg) > 25: msg = msg[:22] + "..." # Max 25 chars long
> +		return "<Changeset @(%s) by %s (%s) updating %i files>" % (
> +			self.date, self.author, msg, len(self.revs))

Similar to the git coding style, this might be better written:

...
if len(msg) > 25:
    msg = msg[:22] + '...' # Max 25 chars long
...

(aka avoid single-line ifs)

There's a few other instances of this in the patch as well.



> diff --git a/git_remote_cvs/cvs.py b/git_remote_cvs/cvs.py
> new file mode 100644
> index 0000000..cc2e13f
> --- /dev/null
> +++ b/git_remote_cvs/cvs.py
> @@ -0,0 +1,884 @@
> [...]
> +
> +	def enumerate (self):
> +		"""Return a list of integer components in this CVS number"""
> +		return list(self.l)

enumerate has special meaning in Python.

items = (1, 2, 3, 4)
for idx, item in enumerate(items):
    print idx, item


I'm not sure if this would cause confusion...


> [...]
> +		else: # revision number
> +			assert self.l[-1] > 0

asserts go away when running with PYTHONOPTIMIZE.

If this is really an error then we should we raise an exception
instead?


> +	@classmethod
> +	def test (cls):
> +		assert cls("1.2.4") == cls("1.2.0.4")

Hmm.. Does it make more sense to use the unittest module?

e.g. self.assertEqual(foo, bar)

> diff --git a/git_remote_cvs/cvs_revision_map.py b/git_remote_cvs/cvs_revision_map.py
> new file mode 100644
> index 0000000..7d7810f
> --- /dev/null
> +++ b/git_remote_cvs/cvs_revision_map.py
> @@ -0,0 +1,362 @@
> +#!/usr/bin/env python
> +
> +"""Functionality for mapping CVS revisions to associated metainformation"""
> +
> +from util import *
> +from cvs  import CvsNum, CvsDate
> +from git  import GitFICommit, GitFastImport, GitObjectFetcher

We definitely need absolute imports here.

'import git' could find the git-python project's git module.


Nonetheless, interesting stuff.


-- 
		David