benny-shotvibe
commented
11 years ago Overall this looks good, I haven't tested it yet, once I do I'll merge it, but there are a few comments I have (mostly about aesthetic issues)
- The length of the commit subject line should be at most 72 characters. This really helps when going over the history, and GitHub cuts of longer commit subjects. I realize that sometimes it can be difficult to come up with a short commit subject, but it's important. I really agree with Tim Pope on this: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
- In your commit there are a few unrelated whitespace formatting changes, these add unnecessary noise that make reviewing code more difficult, and is especially bad when later reviewing history (with tools like git blame)
- There are small unrelated changes in a single commit, they should be in separate commits (.gitignore, addition PIL in requirements.txt)
- All files should end with a single newline character
I will make some inline comments in the commit. I realize that these issues may seem superficial and unimportant, but I think one of the best features of git is that it allows you to keep a clean history, and this makes working with evolving code bases much more pleasant and efficient. Even if this means that committing code takes more time, I think it's worth it overall.
Also, it's probably a good idea to have some good tools that help you do clean commits, the git command line interface is not suitable for my tastes for things like viewing diffs.
So please fix these issues up and then I'll merge (you can do a commit --amend and then force push)
... each user