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

From: Johan Herland <johan@herland.net>
Date: Aug 12, 2009, 09:08 UTC
Message-ID: <200908121108.28714.johan@herland.net>
In-Reply-To: <20090812021017.GB62301@gmail.com>

First, thank you very much for the review. It is very helpful, and I really appreciate it.

On Wednesday 12 August 2009, David Aguilar wrote:

Show 32 quoted lines

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

I'd rather do "from util import X Y Z", as the util stuff is used all over the place.

Show 6 quoted lines

> 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

I thought the python import rules specified that the current package was consulted first, and therefore the 'util' package would always come from the current package. However, I must confess that I don't know these rules very well, so I'll take your word for it and use absolute imports instead.

Show 7 quoted lines

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

Ok. Will change.

Show 5 quoted lines

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

Interesting. I have (obviously) never looked at PEP 8... :)

Show 5 quoted lines

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

I see your point, but I believe that since git_remote_cvs is not an independent project (but very much coupled to git), its allegiance is with Git, and it should therefore follow the Git coding style. In other words, I claim exception (2) in PEP 8

Show 7 quoted lines

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

Yes, I only use __slots__ for classes that potentially have a large number of instances.

Show 9 quoted lines

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

I can't find a good rationale for this rule in PEP8 (other than Guido's personal style), and I personally find the above much more readable (otherwise I wouldn't go through the trouble of lining them all up...). Can I claim exception (1) (readability)?

Show 14 quoted lines

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

Ok. Will try to eliminate single-line ifs.

Show 20 quoted lines

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

Good point, I should probably rename this method.

Show 8 quoted lines

> > [...]
> > +		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?

I use asserts to verify pre/post-conditions and other invariants. I believe that if this assert fails, it is indicative of something horribly wrong with the code itself. However, I now see that one can also trigger this case with bad input (e.g. CvsNum("1.2.3.0").parent()). I will keep the assert here, but will also add some input verification to the CvsNum class.

Show 7 quoted lines

> > +	@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)

Probably. I'm not familiar with 'unittest', but will take a look.

Show 17 quoted lines

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

Ok. Will fix.

> Nonetheless, interesting stuff.

Thanks for the review!

Have fun! :)

...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net