From: Karl Hasselström <kha@treskal.com>
Date: Thu, 05 Jun 2008 07:01:26 GMT
Subject: Re: [StGIT PATCH 3/5] Create a git.Branch class as ancestor of stack.Stack
Message-ID: <20080605070126.GC23209@diana.vm.bytemark.co.uk>
In-Reply-To: <20080604211334.32531.74258.stgit@localhost.localdomain>

On 2008-06-04 22:13:35 +0100, Catalin Marinas wrote:

> This class deals with Git-specific branch commands. The Stack class
> is a direct child of Branch and some of its functionality was moved
> to the new class.

Looks good except for a few minor points.

> @@ -619,3 +626,47 @@ class IndexAndWorktree(RunWithEnvCwd):
>      def update_index(self, files):
>          self.run(['git', 'update-index', '--remove', '-z', '--stdin']
>                   ).input_nulterm(files).discard_output()
> +
> +class Branch(object):
> +    """Represents Git branch."""

Missing "a".

> +    def __init__(self, repository, name):
> +        self._repository = repository
> +        self._name = name

Using double leading underscores will give you a class-local name. Why
just one?

> +    @classmethod
> +    def create(cls, repository, name, create_at = None):

Minor nit: in case of things that are clearly "owned" by other things
(git objects are owned by a git repository, for example), I've tried
to put the creation function in the owner, not the owned. I'm not
insisting, though.

> +        """Create a new Git branch and return the corresponding L{Branch} object."""

Long line.

-- 
Karl Hasselström, kha@treskal.com
      www.treskal.com/kalle