From: Johan Herland <johan@herland•net>
To: David Aguilar <davvid@gmail•com>
Cc: git@vger•kernel.org, barkalow@iabervon•org, gitster@pobox•com,
Johannes.Schindelin@gmx•de
Subject: Re: [RFCv3 2/4] Add Python support library for CVS remote helper
Date: Wed, 12 Aug 2009 11:08:28 +0200 [thread overview]
Message-ID: <200908121108.28714.johan@herland.net> (raw)
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:
> 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.
> 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.
> > +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.
> 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... :)
> 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
> > +
> > + __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.
> > + 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)?
> > + 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.
> > 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.
> > [...]
> > + 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.
> > + @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.
> > 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
next prev parent reply other threads:[~2009-08-12 9:08 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-08-12 0:13 [RFCv3 0/4] CVS remote helper Johan Herland
2009-08-12 0:13 ` [RFCv3 1/4] Basic build infrastructure for Python scripts Johan Herland
2009-08-12 0:13 ` [RFCv3 2/4] Add Python support library for CVS remote helper Johan Herland
2009-08-12 2:10 ` David Aguilar
2009-08-12 9:08 ` Johan Herland [this message]
2009-08-12 17:43 ` Sverre Rabbelier
2009-08-13 0:00 ` Michael Haggerty
2009-08-13 0:20 ` Johan Herland
2009-08-13 0:55 ` Junio C Hamano
2009-08-13 1:27 ` Johan Herland
2009-08-16 19:48 ` Junio C Hamano
2009-08-16 20:38 ` [PATCH 1/2] git_remote_cvs: Honor DESTDIR in the Makefile David Aguilar
2009-08-16 20:38 ` [PATCH 2/2] git_remote_cvs: Use $(shell) " David Aguilar
2009-08-16 20:47 ` David Aguilar
2009-08-16 20:55 ` [PATCH 1/2] git_remote_cvs: Honor DESTDIR " Johannes Schindelin
2009-08-16 21:03 ` David Aguilar
2009-08-16 21:21 ` Johannes Schindelin
2009-08-17 1:58 ` Johan Herland
2009-08-16 21:25 ` [PATCH v2 " David Aguilar
2009-08-16 21:25 ` [PATCH v2 2/2] git_remote_cvs: Use $(shell) " David Aguilar
2009-08-12 0:13 ` [RFCv3 3/4] Third draft of CVS remote helper program Johan Herland
2009-08-12 0:13 ` [RFCv3 4/4] Add simple selftests of git-remote-cvs functionality Johan Herland
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=200908121108.28714.johan@herland.net \
--to=johan@herland$(echo .)net \
--cc=Johannes.Schindelin@gmx$(echo .)de \
--cc=barkalow@iabervon$(echo .)org \
--cc=davvid@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox