Re: Index format v5

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: May 16, 2012, 21:54 UTC
Message-ID: <20120516215407.GA1738@tgummerer.surfnet.iacbox>
In-Reply-To: <4FB334C7.2070201@alum.mit.edu>

On 05/16, Michael Haggerty wrote:

Show 6 quoted lines

> I just reviewed version 1369bd855b86 of your script, and it is MUCH
> better.  It's easy to read and review.  The functions that it
> defines are now self-contained and could therefore be reused for
> other purposes.  There are fewer magic numbers (though there are
> still a few; I wonder if there is a way to get rid of those?)
> You've done a nice job polishing up the code.

Thanks for the feedback! I could get rid of the magic numbers for the crc code, but I'm not sure if it makes sense to replace the others with constants, since they only occur once in the file. I added comments instead explaining where those numbers come from instead.

Show 19 quoted lines

> I have only a few remaining niggles::
> 
> 1. The struct module can handle fixed-length strings, so you could
> read and parse the SHA1s as part of FILE_DATA_STRUCT and
> DIR_DATA_STRUCT rather than handling them separately.
> 
> 2. At least some of the functions deserve docstrings, especially
> when they are nontrivial.  For example, what arguments read_files()
> needs and how they are used is far from obvious.
> 
> 3. It would be easier to read the multiline string formatting
> templates if they are written as multiline strings (even though this
> kindof requires that they be made file-level constants); e.g.,
> 
> FILE_FORMAT = """\
> %(name)s (%(objhash)s)
> mtime: %(mtimes)s:%(mtimens)s
> mode: %(mode)s flags: %(flags)s
> statcrc: """

Thanks, I have changed those. I added the docstrings for all read functions, they however don't seem to make sense for the print functions, since you're probably faster just reading the code for them.

One since I changed in addition is to those changes, is that I gave the exceptions names to make them more meaningful.

Show 10 quoted lines

> In the future, please try to commit one self-contained change at a
> time and make your commit messages really describe what is changed
> in the commit.  For example, commit fb2654c648a does at least three
> things, only one of which is mentioned in its commit message.  It
> would be better to break this into three commits with three commit
> messages.  Use "git rebase -i" and the other commit-rewriting tools
> liberally to tidy up commits before publishing them (but not after
> publishing them!); for example, commit be8d01c22c should really have
> been squashed on top of "Changed main to a function" so that the
> rest of the world doesn't have to see the broken latter commit.

Ok, I'll try my best to do that.

-- Thomas