On 05/15/2012 03:49 PM, Thomas Gummerer wrote:
Thanks again for your feedback. I've refactored the code again,
thanks to your suggestions.
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.
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: """
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.
But I don't want to detract from the fact that altogether you have made
very nice improvements to the script, and I hope to see more like this
from you in the future!
Michael
--
Michael Haggerty
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html