Thanks for submitting this patch.

Before I do a throrough review I'd like to clear up an issue with licensing.
ReviewBot is MIT licensed, and I will only be able to include contributions
if they are under the same license. The license you appear to have chosen is
the BSD license, which is very similar to the MIT license.

What does concern me though is the addition of a clause to the BSD license:

    #  * All advertising materials mentioning features or use of this software must
    # display the following acknowledgement: This product includes software developed
    # by %ORGANIZATION% and its contributors.

I will not be able to accept code with this licensing clause.

If you are willing to relicense the code, I would still love to include it
in the main ReviewBot package. Otherwise, you would have to release these
tools yourself.

Hey Dan, these look good, I'm excited to get them in. Sorry
this review has taken me so long.

There are mainly just some style issues, and some changes that
should be made due to recent improvements to Review Bot (which
will hopefully make things nicer for these tools anyway).

This should be rewritten to say something like:

"... and comes with the following plugins by default: ..."

Once cpplint is added to the install_requires, we shouldn't have to worry about these instructions. You can probably go ahead and remove this.

Remove trailing whitespace

Good idea, I didn't consider some tools would refuse to work on files without an extension.

This should be removed.

After removing the call to logger, this should be removed.

This should be removed as well.

I feel like this is going to be dumping way too much information to the log. Could you remove this please.

Lets get rid of this change. I've updated Review Bot to only send a task when a new diff is posted.

Currently we need to show a review happened even if no files were processed, otherwise there is no way to tell if the review was actually triggered.

Since we're no longer going to kill the review when processed_files is empty, lets remove all these changes.

The base class has changed slightly with the dependency checking as well.

Trailing whitespace should be removed

This can be removed.

blank line between these.

The order should be flipped here (alphabetical)

2 blank lines between these.

This should be 'CPPCheckTool'

How about something like: "Checks code for errors using Cppcheck - A tool for static C/C++ code analysis"

Most of the calls to logger in here should probably call 'logger.debug' instead of 'logger.info'.

Could you update them please.

It might be a good idea to handle the cases where the file extensions are upper or mixed case etc.

Remove trailing whitespace.

I like that you included these example lines, it's a nice addition.

Remove trailing whitespace.

Let's remove this.

This may not do what is expected. Review Bot will open an issue depending on global settings configured in the admin panel. When you're passing 'issue=True' you are overriding this behaviour, and ensuring an issue is made.

In the else case, you don't pass any value for issue, so you are letting Review Bot decide if an issue should be raised based on the admin configuration.

If you really want to override Review Bots issue decisions, you should probably also pass 'issue=False'.

Remove trailing whitespace.

this can be removed.

blank line between these

flip the order of these (alphabetical)

This should be 'CPPLintTool'

Why the '(Modified by E///)' part?

let's add 'max_value', and 'min_value' of 5 and 1 respectively.

Most of these should be changed to logger.debug as well.

might want to handle upper or mixed case as well.

I've added the ability for tools to specify a 'check_dependencies' method which can carry out checks like this. If the method returns false, that tool won't be activated for the worker.

Lets move these checks into methods on their respective tools.

I've added an AUTHORS file to the repo where you may add your name, this should remain unchanged.

Can you put these in alphabetical order.

You should be able to add cpplint to this list, and it will be automatically installed with Review Bot.

Lets axe the dependency on distutils2. I've added a 'is_exe_in_path' function which you can use to check for the tools you call out to.

Alright, so I took a look at your github fork of Review Bot, and it looks like
the reason you're getting my changes in your review requests is because you've
been committing to the master branch.

I'm going to assume going forward that your local repository was cloned from
the main Review Bot repository, so the origin remote refers to
git://github.com/smacleod/ReviewBot.git. Also, <fork> should refer to the
name you gave the remote for your github fork of Review Bot.

Your development should take place on a separate branch, which I'll refer to
as <feature> from this point forward. As you make changes, you should be
committing to the <feature> branch. When you are ready to update your review
request, you should merge the changes from master into your <feature> branch,
and call post-review with <feature> checked out. ex:

    $ git checkout master
    $ git fetch origin
    $ git merge origin/master
    $ git checkout <feature>
    $ git merge master
    $ post-review -r 3579

Before this will work though, we need to fix the master branch on your repo.
So, what we need to do is move your changes into a <feature> branch, and reset
your master to origin/master. We'll also force push these changes to your
fork.

First Create the <feature> branch:
    $ git checkout master
    $ git branch <feature>

Then reset your master branch:
    $ git reset --hard origin/master

Push the changes to your fork:
    $ git push --force <fork> master

Now, you'll want to properly base your changes off master:
    $ git checkout <feature>
    $ git rebase master

You might get conflicts when rebasing here, which you'll have to resolve.
After this is finished, you should be in a good state to update the
review request.

Let me know if you have any problems.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/tools/cpplint.py
    bot/setup.py
    bot/reviewbot/processing/filesystem.py
    bot/reviewbot/tools/cppcheck.py
    bot/reviewbot/processing/review.py
  Ignored Files:
    README.md
    AUTHORS


WARNING: Number of comments exceeded maximum, showing 30 of 35.

Col: 80
 E501 line too long (85 > 79 characters)

Col: 80
 E501 line too long (97 > 79 characters)

Col: 80
 E501 line too long (137 > 79 characters)

Col: 80
 E501 line too long (107 > 79 characters)

Col: 9
 E303 too many blank lines (2)

Col: 80
 E501 line too long (84 > 79 characters)

Col: 80
 E501 line too long (86 > 79 characters)

Col: 80
 E501 line too long (85 > 79 characters)

Col: 42
 E261 at least two spaces before inline comment

Col: 43
 E262 inline comment should start with '# '

Col: 80
 E501 line too long (137 > 79 characters)

Col: 80
 E501 line too long (131 > 79 characters)

Col: 80
 E501 line too long (132 > 79 characters)

Col: 80
 E501 line too long (80 > 79 characters)

Col: 80
 E501 line too long (92 > 79 characters)

Col: 80
 E501 line too long (127 > 79 characters)

Col: 14
 E111 indentation is not a multiple of four

Col: 17
 E121 continuation line indentation is not a multiple of four

Col: 17
 E121 continuation line indentation is not a multiple of four

Col: 17
 E121 continuation line indentation is not a multiple of four

Col: 80
 E501 line too long (100 > 79 characters)

Col: 80
 E501 line too long (101 > 79 characters)

Col: 80
 E501 line too long (100 > 79 characters)

Col: 80
 E501 line too long (122 > 79 characters)

Col: 43
 E262 inline comment should start with '# '

Col: 80
 E501 line too long (88 > 79 characters)

Col: 50
 E262 inline comment should start with '# '

Col: 80
 E501 line too long (88 > 79 characters)

Col: 51
 E262 inline comment should start with '# '

Col: 80
 E501 line too long (88 > 79 characters)

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/tools/cpplint.py
    bot/setup.py
    bot/reviewbot/processing/filesystem.py
    bot/reviewbot/tools/cppcheck.py
    bot/reviewbot/processing/review.py
  Ignored Files:
    README.md
    AUTHORS

Col: 80
 E501 line too long (85 > 79 characters)

Thanks Dan!