Summary

Setting up the bot to handle Git repositories and pass the repository paths to the tools

Review Request #5869 — Created May 23, 2014 and discarded Jan. 29, 2019, 1:39 p.m.

Information

Owner

matthewcmaclean

Repository

ReviewBot

Branch

release-0.2.x

Bugs

Depends On

Reviewers

Groups

reviewbot, students

People

Description

Sets up repository on ReviewBot load

- stores repositories in root folder

- updates repositories programatically  
Supports Git, but easy to extend for new types of repositories

- switch to base commit

- use SCM tool to apply patch to temp repository
Fixed bug in make_tempfile
Delete temporary folders
Added cached_property to ReviewBot

- turned File properties into cached properties

- cached path property on Repository_Clone
Added new Tool base class to support access to entire repository

- sending repository path to tool

- comment on a file by its path

- created example ReviewBot tool to look at entire repository

- added more Queues to Celery so that a bot only picks up a task that it has the repository for  
Added flag to extension model to signal if ReviewBot tool supports entire directory

- added UI in admin panel to show this

Testing Done

The following configuration was used in reviewbot/config.py
from reviewbot.revisioning.constants import GIT


BOT_REPOSITORY_SETTINGS = {
    'http://localhost:8080/': {
        'ReviewBot': {
            'type': GIT,
            'url': 'git://github.com/reviewboard/ReviewBot.git',
        },
    },
}


The following tool was added at the path reviewbot/tools/pep8repo.py
import os

from reviewbot.tools import RepositoryTool
from reviewbot.tools.process import execute
from reviewbot.utils import is_exe_in_path


class PEP8RepoTool(RepositoryTool):
    name = 'PEP8 Style Checker - Entire Repository'
    version = '0.2'
    description = "Checks code for style errors using the PEP8 tool."

    def check_dependencies(self):
        return is_exe_in_path('pep8')

    def handle_repository(self, repository_working_directory):
        path = repository_working_directory.path
        if not path:
            return False

        for root, subFolders, files in os.walk(path):
            for file_path in files:
                if not file_path.endswith('.py'):
                    continue

                full_path = os.path.join(root, file_path)

                output = execute(
                    [
                        'pep8',
                        '-r',
                        full_path
                    ],
                    split_lines=True,
                    ignore_errors=True)

                for line in output:
                    parsed = line.split(':', 3)
                    lnum = int(parsed[1])
                    col = int(parsed[2])
                    msg = parsed[3]
                    repository_working_directory.comment(
                        full_path,
                        'Col: %s\n%s' % (col, msg),
                        lnum)

        return True


I used a server running on "http://localhost:8080" to test my changes. The setup process is the same as if you were setting up ReviewBot normally. When refreshing the list of tools, PEP8 Style Checker - Entire Repository should show up with a value of true in the 'working directory required' column. To test I set both it and the normal PEP8 Style Checker to run automatically to compare output.
Tests:

1. On the second startup of the bot, it should take less time than the second to start Celery (if you include the reviewboard repository it is more noticeable). This indicates that the repository is being stored between bot sessions as expected. These repositories should stored in a folder system ~/ReviewBotRepositories/server_url/repo_name/.

2. In the celery startup output, it should be shown that one of the Celery Queues the bot is listening on is: pep8repo.0.2.http://localhost:8080/.ReviewBot exchange=pep8repo.0.2.http://localhost:8080/.ReviewBot(direct) key=pep8repo.0.2.http://localhost:8080/.ReviewBot

3. The reviewboard page /admin/extensions/reviewbotext.extension.ReviewBotExtension/db/reviewbotext/reviewbottool/ should show a column called Working directory required. For the tool 'PEP8 Style Checker - Entire Repository' there should be a green check.

5. In the middle of a review being made (celery.contrib.rdb.set_trace() was used to pause in the middle), there should be a file in /tmp/ that contains the patch to apply to the repository to get the code as it is in the review request.

4. In the middle of a review being made (celery.contrib.rdb.set_trace() was used to pause in the middle), there should be a folder in /tmp/ that contains the ReviewBot repository without the .git folder. The state of the repository should be identical to that of the review request.

5. After a publishing a review, checking the /tmp/ folder of the bot should show there are no temp files or directories.

6. Both the normal PEP8 and the full repository PEP8 should complete without error. The resulting code comments should be identical. Of particular note, there should be no comments on files not changed in the review request.

Issues

Description	From	Last Updated
Col: 14 E203 whitespace before ':'	reviewbot	June 9, 2014, 11:11 a.m.
Col: 14 E203 whitespace before ':'	reviewbot	June 9, 2014, 11:11 a.m.
local variable 'repo' is assigned to but never used	reviewbot	June 9, 2014, 11:12 a.m.
Col: 15 E203 whitespace before ':'	reviewbot	June 9, 2014, 11:12 a.m.
Col: 14 E203 whitespace before ':'	reviewbot	June 9, 2014, 11:12 a.m.
Col: 15 E203 whitespace before ':'	reviewbot	June 9, 2014, 11:12 a.m.
Col: 14 E203 whitespace before ':'	reviewbot	June 9, 2014, 11:12 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	June 9, 2014, 11:12 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	June 9, 2014, 11:12 a.m.
local variable 'repo' is assigned to but never used	reviewbot	June 9, 2014, 11:15 a.m.
'Repo' imported but unused	reviewbot	June 9, 2014, 11:15 a.m.
'NoSuchPathError' imported but unused	reviewbot	June 9, 2014, 11:15 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	June 9, 2014, 11:12 a.m.
Col: 80 E501 line too long (99 > 79 characters)	reviewbot	June 9, 2014, 11:12 a.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	June 9, 2014, 11:12 a.m.
Col: 52 E251 unexpected spaces around keyword / parameter equals	reviewbot	June 9, 2014, 11:12 a.m.
Col: 54 E251 unexpected spaces around keyword / parameter equals	reviewbot	June 9, 2014, 11:12 a.m.
Col: 51 E251 unexpected spaces around keyword / parameter equals	reviewbot	June 9, 2014, 11:12 a.m.
Col: 53 E251 unexpected spaces around keyword / parameter equals	reviewbot	June 9, 2014, 11:12 a.m.
Col: 71 E251 unexpected spaces around keyword / parameter equals	reviewbot	June 9, 2014, 11:12 a.m.
Col: 73 E251 unexpected spaces around keyword / parameter equals	reviewbot	June 9, 2014, 11:12 a.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	June 9, 2014, 11:12 a.m.
Col: 1 W391 blank line at end of file	reviewbot	June 9, 2014, 11:12 a.m.
'ToolRepository' imported but unused	reviewbot	June 9, 2014, 11:15 a.m.
Col: 5 E265 block comment should start with '# '	reviewbot	June 9, 2014, 11:12 a.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	June 9, 2014, 11:12 a.m.
Col: 50 E261 at least two spaces before inline comment	reviewbot	June 9, 2014, 11:12 a.m.
Col: 50 E262 inline comment should start with '# '	reviewbot	June 9, 2014, 11:12 a.m.
Col: 5 E265 block comment should start with '# '	reviewbot	June 9, 2014, 11:12 a.m.
Col: 80 E501 line too long (82 > 79 characters)	reviewbot	June 14, 2014, 10:36 p.m.
'RepositoryManager' imported but unused	reviewbot	June 14, 2014, 10:57 p.m.
local variable 'tool_repository' is assigned to but never used	reviewbot	June 21, 2014, 2:38 p.m.
Col: 22 E251 unexpected spaces around keyword / parameter equals	reviewbot	June 14, 2014, 10:57 p.m.
Col: 24 E251 unexpected spaces around keyword / parameter equals	reviewbot	June 14, 2014, 10:57 p.m.
Col: 29 E203 whitespace before ':'	reviewbot	June 14, 2014, 10:57 p.m.
local variable 'tool_repository' is assigned to but never used	reviewbot	June 21, 2014, 2:38 p.m.
You'll want to put a blank line before from reviewboar.revisioning.repository_manager import RepositoryManger. The why we style our imports is as …	SM smacleod	June 25, 2014, 1:09 p.m.
Leave this blank line in, we want 2 blanks here.	SM smacleod	June 25, 2014, 1:10 p.m.
So as I mentioned in the hackpad we want to namespace the configuration down one more level to include the …	SM smacleod	June 25, 2014, 1:29 p.m.
Is there a reason you chose to use abstract base classes for this? If not, I think we should just …	SM smacleod	June 25, 2014, 1:48 p.m.
The docstring should be before any code in the method.	SM smacleod	June 25, 2014, 1:14 p.m.
What is _path here suppose to represent, and why does it just return self.name? What am I missing?	SM smacleod	June 25, 2014, 1:14 p.m.
So a couple of things with this: @classmethod methods should generally use "cls" as the first argument, not "self". What …	SM smacleod	June 26, 2014, 11:32 a.m.
Instead of doing these checks from constants and then using a particular class, we should just create a dictionary mapping …	SM smacleod	June 25, 2014, 1:23 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	June 25, 2014, 1:07 p.m.
In order to intiialize the tool repository we'll need: * The name of the repository * The Review Board server …	SM smacleod	July 12, 2014, 5:13 p.m.
local variable 'tool_repository' is assigned to but never used	reviewbot	July 12, 2014, 5:13 p.m.
Lets call this working_dir_required	SM smacleod	June 25, 2014, 1:15 p.m.
'RepositoryManager' imported but unused	reviewbot	July 12, 2014, 5:12 p.m.
undefined name 'shutil'	reviewbot	July 12, 2014, 5:12 p.m.
Col: 1 E302 expected 2 blank lines, found 1	reviewbot	July 12, 2014, 5:12 p.m.
Col: 1 W391 blank line at end of file	reviewbot	July 12, 2014, 5:12 p.m.
local variable 'e' is assigned to but never used	reviewbot	July 12, 2014, 5:12 p.m.
undefined name 'logging'	reviewbot	July 12, 2014, 5:12 p.m.
Col: 25 E128 continuation line under-indented for visual indent	reviewbot	July 12, 2014, 5:12 p.m.
'rdb' imported but unused	reviewbot	July 18, 2014, 1:12 p.m.
Col: 9 E265 block comment should start with '# '	reviewbot	July 18, 2014, 1:12 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	July 18, 2014, 1:12 p.m.
undefined name 'self'	reviewbot	July 18, 2014, 1:12 p.m.
Col: 29 E126 continuation line over-indented for hanging indent	reviewbot	July 18, 2014, 1:12 p.m.
Col: 17 E128 continuation line under-indented for visual indent	reviewbot	July 18, 2014, 1:12 p.m.
undefined name 'path'	reviewbot	July 23, 2014, 7:42 p.m.
undefined name 'payload'	reviewbot	July 23, 2014, 7:42 p.m.
undefined name 'logger'	reviewbot	July 23, 2014, 7:42 p.m.
undefined name 'repository_manager'	reviewbot	July 23, 2014, 7:42 p.m.
undefined name 'payload'	reviewbot	July 23, 2014, 7:42 p.m.
undefined name 'RepositoryClone'	reviewbot	July 23, 2014, 7:42 p.m.
undefined name 'logger'	reviewbot	July 23, 2014, 7:42 p.m.
undefined name 'repository_clone'	reviewbot	July 23, 2014, 7:42 p.m.
Col: 80 E501 line too long (85 > 79 characters)	reviewbot	July 23, 2014, 7:42 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	July 23, 2014, 7:42 p.m.
Col: 21 E128 continuation line under-indented for visual indent	reviewbot	July 23, 2014, 7:42 p.m.
'RepositoryClone' imported but unused	reviewbot	July 23, 2014, 7:42 p.m.
Col: 19 E262 inline comment should start with '# '	reviewbot	July 23, 2014, 7:42 p.m.
'GIT' imported but unused	reviewbot	Aug. 1, 2014, 4:32 p.m.
Maybe I should include this in the comments rather than in the actual code. I really want whoever writes the …	MA matthewcmaclean	Aug. 1, 2014, 4:32 p.m.
Can you remove the extra space and add a period at the end of the sentence?	anselina	Aug. 4, 2014, 9:11 a.m.
Capitalize the first word + period (for consistency?)	PE PeterTran	Aug. 4, 2014, 9:08 a.m.
s/supoort/support	anselina	Aug. 4, 2014, 9:12 a.m.
s/an/An	anselina	Aug. 4, 2014, 9:12 a.m.
Can you capitalize URL? Here and in all your other comments/docstrings.	anselina	Aug. 4, 2014, 9:16 a.m.
Remove the apostrophe on "name's".	anselina	Aug. 4, 2014, 9:17 a.m.
We generally add full-package imports before single-item imports, so this should go after import tempfile.	anselina	Aug. 4, 2014, 9:17 a.m.
Remove the extra space before and after the sentence, and add a period.	anselina	Aug. 4, 2014, 9:18 a.m.
Can you add a blank line after this? We generally have one blank line before and after if/for/while blocks.	anselina	Aug. 4, 2014, 9:18 a.m.
Can you add a blank line before this, while you're here?	anselina	Aug. 4, 2014, 9:18 a.m.
Add a blank line before this.	anselina	Aug. 4, 2014, 9:18 a.m.
Remove the extra space before the sentence.	anselina	Aug. 4, 2014, 9:19 a.m.
Remove the extra space before and after the sentence.	anselina	Aug. 4, 2014, 9:19 a.m.
Remove the extra space before and after the sentence. Here and in all other docstrings below.	anselina	Aug. 4, 2014, 9:21 a.m.
Remove the extra space before the sentence, and add a period.	anselina	Aug. 4, 2014, 9:21 a.m.
This should come directly after from git.exc import NoSuchPathError (since RBTools would be considered as a third-party library import for …	anselina	Aug. 4, 2014, 9:22 a.m.
This should be: from os import chdir, get cwd from re import sub from django.utils.functional import cached_property from rbtools.commands.patch import …	anselina	Aug. 4, 2014, 9:24 a.m.
We generally use single quotes for strings. (Here and in other places.)	anselina	Aug. 4, 2014, 9:40 a.m.
Can you change this to: logging.warning( 'server: %s, repository %s: type%s is not supported' % (server_url, repo_name, repo_settings['type']))	anselina	Aug. 4, 2014, 9:26 a.m.
You will need a space after been. Otherwise when the Exception is raised it'll be concatenated like: Respository _ in …	PE PeterTran	Aug. 4, 2014, 9:09 a.m.
Can you make this into a single-line docstring, like """Returns the server URL and names of repositories the bot supports."""?	anselina	Aug. 4, 2014, 9:26 a.m.
s/seperately/separately	anselina	Aug. 4, 2014, 9:27 a.m.
Should this class be uppercase?	PE PeterTran	Aug. 4, 2014, 9:09 a.m.
Can you remove the extra space before the sentence, and add a period?	anselina	Aug. 4, 2014, 9:27 a.m.
Add a blank line after this.	anselina	Aug. 4, 2014, 9:28 a.m.
Since you removed your pep8repo tool used for testing, this line should also be deleted.	anselina	Aug. 4, 2014, 9:28 a.m.
If there are multiple tools with working_directory_required == True, this will initialize the RepositoryManager multiple times. Each queue would be …	anselina	Aug. 6, 2014, 9:21 a.m.
If the patch doesn't apply cleanly or raises an error, we probably shouldn't proceed with the review. execute() has some …	anselina	Aug. 9, 2014, 9:45 p.m.
Can we call this RepositoryWorkingDirectory instead (or something to that effect), and change the docstring to "Represents a working directory …	anselina	Aug. 9, 2014, 1:52 p.m.
Can we put this in a function like initialize_repository_manager(), and just add a call to this in celery.py if a …	anselina	Aug. 9, 2014, 9:09 p.m.
Can you indent this an extra level?	anselina	Aug. 9, 2014, 1:32 p.m.
"static" should be removed since we can now do more than just static analysis with the full working directory.	anselina	Aug. 9, 2014, 1:31 p.m.
Indent this an extra level.	anselina	Aug. 9, 2014, 1:31 p.m.
Actually, sorry, instead of calling initialize_repositories() here, how about we call it in supported_repositories()?	anselina	Aug. 9, 2014, 9:09 p.m.
Globals are usually capitalized, so can you make this be REPOSITORIES?	anselina	Aug. 9, 2014, 9:09 p.m.
Let's set REPOSITORIES = None in line 12, and set REPOSITORIES = {} here. This way, we can just do …	anselina	Aug. 9, 2014, 9:09 p.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/config.py
  Ignored Files:

bot/reviewbot/config.py (Diff revision 1)
Show all issues
```
Col: 14
 E203 whitespace before ':'
```
bot/reviewbot/config.py (Diff revision 1)
Show all issues
```
Col: 14
 E203 whitespace before ':'
```

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/config.py
  Ignored Files:

bot/reviewbot/tasks.py (Diff revision 1)

Show all issues

 local variable 'repo' is assigned to but never used

Description:

~		I'm currently exploring GitPython and how I can use it to manipulate the repositories for the different tools.
	~	Setting up the bot to handle Git repositories and pass the file paths to the tools.
	+	Currently it:
	+	- sets up repository on load
	+	- supports Git, but easy to extend for new types of repositories
	+	- stores repository in root
	+	WIP:
	+	- sending file path to tools

Commit:

-	22b59a38ea81e80902e538afbccc328785d6bc77
+	7bdbc0ffd92332411cf46fd716172e776a6b0927

Diff:

Revision 2 (+109 -2)

Show changes

	bot/setup.py
	bot/reviewbot/celery.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/revisioning/base_repository.py
	bot/reviewbot/revisioning/constants.py
	bot/reviewbot/revisioning/git_repository.py
	bot/reviewbot/revisioning/repository_manager.py
	2 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/revisioning/base_repository.py
    bot/setup.py
    bot/reviewbot/revisioning/tool_repository.py
    bot/reviewbot/celery.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/config.py
  Ignored Files:

bot/reviewbot/config.py (Diff revision 2)
Show all issues
```
Col: 15
 E203 whitespace before ':'
```
bot/reviewbot/config.py (Diff revision 2)
Show all issues
```
Col: 14
 E203 whitespace before ':'
```
bot/reviewbot/config.py (Diff revision 2)
Show all issues
```
Col: 15
 E203 whitespace before ':'
```
bot/reviewbot/config.py (Diff revision 2)
Show all issues
```
Col: 14
 E203 whitespace before ':'
```
bot/reviewbot/revisioning/base_repository.py (Diff revision 2)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
bot/reviewbot/revisioning/git_repository.py (Diff revision 2)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
bot/reviewbot/revisioning/repository_manager.py (Diff revision 2)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
bot/reviewbot/revisioning/repository_manager.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (99 > 79 characters)
```
bot/reviewbot/revisioning/tool_repository.py (Diff revision 2)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
bot/reviewbot/revisioning/tool_repository.py (Diff revision 2)
Show all issues
```
Col: 52
 E251 unexpected spaces around keyword / parameter equals
```
bot/reviewbot/revisioning/tool_repository.py (Diff revision 2)
Show all issues
```
Col: 54
 E251 unexpected spaces around keyword / parameter equals
```
bot/reviewbot/revisioning/tool_repository.py (Diff revision 2)
Show all issues
```
Col: 51
 E251 unexpected spaces around keyword / parameter equals
```
bot/reviewbot/revisioning/tool_repository.py (Diff revision 2)
Show all issues
```
Col: 53
 E251 unexpected spaces around keyword / parameter equals
```
bot/reviewbot/revisioning/tool_repository.py (Diff revision 2)
Show all issues
```
Col: 71
 E251 unexpected spaces around keyword / parameter equals
```
bot/reviewbot/revisioning/tool_repository.py (Diff revision 2)
Show all issues
```
Col: 73
 E251 unexpected spaces around keyword / parameter equals
```
bot/reviewbot/revisioning/tool_repository.py (Diff revision 2)
Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```
bot/reviewbot/revisioning/tool_repository.py (Diff revision 2)
Show all issues
```
Col: 1
 W391 blank line at end of file
```

bot/reviewbot/tasks.py (Diff revision 2)

Show all issues

Col: 5
 E265 block comment should start with '# '

bot/reviewbot/tasks.py (Diff revision 2)

Show all issues

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

bot/reviewbot/tasks.py (Diff revision 2)

Show all issues

Col: 50
 E261 at least two spaces before inline comment

bot/reviewbot/tasks.py (Diff revision 2)

Show all issues

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

extension/reviewbotext/models.py (Diff revision 2)
Show all issues
```
Col: 5
 E265 block comment should start with '# '
```

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/revisioning/base_repository.py
    bot/setup.py
    bot/reviewbot/revisioning/tool_repository.py
    bot/reviewbot/celery.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/config.py
  Ignored Files:

bot/reviewbot/revisioning/git_repository.py (Diff revision 2)
Show all issues
```
 local variable 'repo' is assigned to but never used
```
bot/reviewbot/revisioning/repository_manager.py (Diff revision 2)
Show all issues
```
 'Repo' imported but unused
```
bot/reviewbot/revisioning/repository_manager.py (Diff revision 2)
Show all issues
```
 'NoSuchPathError' imported but unused
```
bot/reviewbot/tasks.py (Diff revision 2)
Show all issues
```
 'ToolRepository' imported but unused
```

Change Summary:

Fixed style issues

Commit:

-	7bdbc0ffd92332411cf46fd716172e776a6b0927
+	02ce4c150c4a9f699a1ff37c5238cd9bdb6b7f68

Diff:

Revision 3 (+117 -2)

Show changes

	bot/setup.py
	bot/reviewbot/celery.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/revisioning/base_repository.py
	bot/reviewbot/revisioning/constants.py
	bot/reviewbot/revisioning/git_repository.py
	bot/reviewbot/revisioning/repository_manager.py
	2 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/revisioning/base_repository.py
    bot/setup.py
    bot/reviewbot/revisioning/tool_repository.py
    bot/reviewbot/celery.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/config.py
  Ignored Files:

bot/reviewbot/tasks.py (Diff revision 3)

Show all issues

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

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/revisioning/base_repository.py
    bot/setup.py
    bot/reviewbot/revisioning/tool_repository.py
    bot/reviewbot/celery.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/config.py
  Ignored Files:

Description:

		Setting up the bot to handle Git repositories and pass the file paths to the tools.
		Currently it:
		- sets up repository on load
		- supports Git, but easy to extend for new types of repositories
		- stores repository in root
		WIP:
	+	- determining information about base commit
		- sending file path to tools

Commit:

-	02ce4c150c4a9f699a1ff37c5238cd9bdb6b7f68
+	f0c674149db9280c5b9aeaececae91a4d2dba557

Diff:

Revision 4 (+120 -1)

Show changes

	bot/setup.py
	bot/reviewbot/celery.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/revisioning/base_repository.py
	bot/reviewbot/revisioning/constants.py
	bot/reviewbot/revisioning/git_repository.py
	bot/reviewbot/revisioning/repository_manager.py
	3 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/revisioning/base_repository.py
    bot/setup.py
    bot/reviewbot/revisioning/tool_repository.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/config.py
  Ignored Files:

bot/reviewbot/tasks.py (Diff revision 4)

Show all issues

Col: 22
 E251 unexpected spaces around keyword / parameter equals

bot/reviewbot/tasks.py (Diff revision 4)

Show all issues

Col: 24
 E251 unexpected spaces around keyword / parameter equals

extension/reviewbotext/handlers.py (Diff revision 4)
Show all issues
```
Col: 29
 E203 whitespace before ':'
```

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/revisioning/base_repository.py
    bot/setup.py
    bot/reviewbot/revisioning/tool_repository.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/config.py
  Ignored Files:

bot/reviewbot/revisioning/tool_repository.py (Diff revision 4)
Show all issues
```
 'RepositoryManager' imported but unused
```

bot/reviewbot/tasks.py (Diff revision 4)

Show all issues

 local variable 'tool_repository' is assigned to but never used

Change Summary:

Fixing reviewbot issues

Commit:

-	f0c674149db9280c5b9aeaececae91a4d2dba557
+	132a80bd76bbdffbffd4ded6945031a21e86d0d3

Diff:

Revision 5 (+117 -1)

Show changes

	bot/setup.py
	bot/reviewbot/celery.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/revisioning/base_repository.py
	bot/reviewbot/revisioning/constants.py
	bot/reviewbot/revisioning/git_repository.py
	bot/reviewbot/revisioning/repository_manager.py
	3 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/revisioning/base_repository.py
    bot/setup.py
    bot/reviewbot/revisioning/tool_repository.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/config.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/revisioning/base_repository.py
    bot/setup.py
    bot/reviewbot/revisioning/tool_repository.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/config.py
  Ignored Files:

bot/reviewbot/tasks.py (Diff revision 5)

Show all issues

 local variable 'tool_repository' is assigned to but never used

Change Summary:

Added base repository commit and repository name to payload

Description:

		Setting up the bot to handle Git repositories and pass the file paths to the tools.
		Currently it:
		- sets up repository on load
		- supports Git, but easy to extend for new types of repositories
		- stores repository in root
-		WIP:
		- determining information about base commit
	+	WIP:
		- sending file path to tools

Commit:

-	132a80bd76bbdffbffd4ded6945031a21e86d0d3
+	81d2d97c1ff079334f453f98d2233d46d5162a92

Diff:

Revision 6 (+137 -25)

Show changes

	bot/setup.py
	bot/reviewbot/.config.py.swp
	bot/reviewbot/celery.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/revisioning/.repository_manager.py.swp
	bot/reviewbot/revisioning/base_repository.py
	bot/reviewbot/revisioning/constants.py
	6 more

Tool: Pyflakes
Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/revisioning/base_repository.py
    bot/setup.py
    bot/reviewbot/revisioning/tool_repository.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/config.py

Ignored Files:
    bot/reviewbot/revisioning/.repository_manager.py.swp
    extension/reviewbotext/.handlers.py.swp
    bot/reviewbot/.config.py.swp



Tool: PEP8 Style Checker
Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/revisioning/base_repository.py
    bot/setup.py
    bot/reviewbot/revisioning/tool_repository.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/config.py

Ignored Files:
    bot/reviewbot/revisioning/.repository_manager.py.swp
    extension/reviewbotext/.handlers.py.swp
    bot/reviewbot/.config.py.swp

bot/reviewbot/revisioning/tool_repository.py (Diff revision 6)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```

bot/reviewbot/tasks.py (Diff revision 6)

Show all issues

 local variable 'tool_repository' is assigned to but never used

I think we're moving in the right directions here. I've done a quick pass and noted some of the style issues I caught, as well as some of the bigger conventional, architectural, or preferential issues.

Also, it looks like a bunch of binary swap files snuck into your diff. Please remove these from your commits in future revisions.

bot/reviewbot/celery.py (Diff revision 6)

Show all issues

You'll want to put a blank line before from reviewboar.revisioning.repository_manager import RepositoryManger.
The why we style our imports is as follows:
<Python Standard Library Imports>

<Blank Line>

<Third Party Library Imports>

<Blank Line>

<Package Imports>
Where third party library imports are things from outside the project (anything not from "reviewbot")

bot/reviewbot/celery.py (Diff revision 6)

Show all issues

Leave this blank line in, we want 2 blanks here.

bot/reviewbot/config.py (Diff revision 6)

Show all issues

So as I mentioned in the hackpad we want to namespace the configuration down one more level to include the Review Board server url.
So in this case we'd have:

    repo_settings = {

        "http://localhost:8080/": {

            "RBTools": {

                "type": GIT,

                "url": "git://github.com/reviewboard/rbtools.git",

            },

            "Review Board": {

                "type": GIT,

                "url": "git://github.com/reviewboard/reviewboard.git",

            },

        },

    }

bot/reviewbot/revisioning/base_repository.py (Diff revision 6)

Show all issues

Is there a reason you chose to use abstract base classes for this?
If not, I think we should just use the normal inheritance. Just defining a set of methods in the base class which make up the interace and then defining them in each class should be fine.

bot/reviewbot/revisioning/base_repository.py (Diff revision 6)
Show all issues
```
The docstring should be before any code in the method.
```

bot/reviewbot/revisioning/base_repository.py (Diff revision 6)

Show all issues

What is _path here suppose to represent, and why does it just return self.name? What am I missing?

bot/reviewbot/revisioning/repository_manager.py (Diff revision 6)

Show all issues

So a couple of things with this:
@classmethod methods should generally use "cls" as the first argument, not "self". What is actually passed will be the class not an object so this makes that clearer.
Using this sort of singleton class as an object is less testable than just actually using an object or sets of functions - and we usually don't do it this way around the rest of the code. (I know, Review Bot doesn't have test... but I hope it does one day).

bot/reviewbot/revisioning/repository_manager.py (Diff revision 6)

Show all issues

Instead of doing these checks from constants and then using a particular class, we should just create a dictionary mapping repositories type string to the classes. That will allow a quick lookup and instantiation of the proper class.
All of these repo types should inherit from the same class and have the same interface going forward. So while there is only git right now, keep that in mind.

bot/reviewbot/tasks.py (Diff revision 6)

Show all issues

In order to intiialize the tool repository we'll need: * The name of the repository * The Review Board server URL,

I think we should keep the commit information out of it at the initialization step, and move that into a method you call on the ToolRepository object - "getWorkingdirectoryForCommit()" or something.

extension/reviewbotext/models.py (Diff revision 6)
Show all issues
```
Lets call this working_dir_required
```

Groups:

-	reviewboard
+	reviewbot

Change Summary:

Create temporary folder with base commit folder.

Description:

		Setting up the bot to handle Git repositories and pass the file paths to the tools.
~		Currently it:
	~	Currently in:
		- sets up repository on load
		- supports Git, but easy to extend for new types of repositories
		- stores repository in root
		- determining information about base commit
	+	- switch to base commit
		WIP:
~		- sending file path to tools
	~	- sending file path to tools
	+	- might not be deleting temporary directory

Commit:

-	81d2d97c1ff079334f453f98d2233d46d5162a92
+	a09494d997de85ce230169cf3e18e8fb5e007943

Diff:

Revision 7 (+211 -25)

Show changes

	bot/setup.py
	bot/reviewbot/celery.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/processing/filesystem.py
	bot/reviewbot/revisioning/base_repository.py
	bot/reviewbot/revisioning/constants.py
	bot/reviewbot/revisioning/git_repository.py
	4 more

Tool: PEP8 Style Checker
Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/revisioning/base_repository.py
    bot/reviewbot/processing/filesystem.py
    bot/setup.py
    bot/reviewbot/revisioning/tool_repository.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/config.py

bot/reviewbot/processing/filesystem.py (Diff revision 7)
Show all issues
```
Col: 1
 E302 expected 2 blank lines, found 1
```
bot/reviewbot/processing/filesystem.py (Diff revision 7)
Show all issues
```
Col: 1
 W391 blank line at end of file
```
bot/reviewbot/revisioning/repository_manager.py (Diff revision 7)
Show all issues
```
Col: 25
 E128 continuation line under-indented for visual indent
```

Tool: Pyflakes
Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/revisioning/base_repository.py
    bot/reviewbot/processing/filesystem.py
    bot/setup.py
    bot/reviewbot/revisioning/tool_repository.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/config.py

bot/reviewbot/celery.py (Diff revision 7)
Show all issues
```
 'RepositoryManager' imported but unused
```
bot/reviewbot/processing/filesystem.py (Diff revision 7)
Show all issues
```
 undefined name 'shutil'
```
bot/reviewbot/revisioning/base_repository.py (Diff revision 7)
Show all issues
```
 local variable 'e' is assigned to but never used
```
bot/reviewbot/revisioning/repository_manager.py (Diff revision 7)
Show all issues
```
 undefined name 'logging'
```

Description:

		Setting up the bot to handle Git repositories and pass the file paths to the tools.
		Currently in:
		- sets up repository on load
		- supports Git, but easy to extend for new types of repositories
		- stores repository in root
		- determining information about base commit
		- switch to base commit
	+	- cached path property
	+	- fixed bug in make_tempfile
	+	- delete temporary folders
	+	- update local repository programatically
		WIP:
		- sending file path to tools
~		- might not be deleting temporary directory
	~	- use SCM tool to apply patch to repository

Commit:

-	a09494d997de85ce230169cf3e18e8fb5e007943
+	f0e81766a7717b6d90b51739d27ea39d737f56e2

Diff:

Revision 8 (+282 -25)

Show changes

	bot/setup.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/processing/filesystem.py
	bot/reviewbot/revisioning/constants.py
	bot/reviewbot/revisioning/git_repository_client.py
	bot/reviewbot/revisioning/repository_client.py
	bot/reviewbot/revisioning/repository_clone.py
	3 more

Tool: PEP8 Style Checker
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py



Tool: Pyflakes
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

bot/reviewbot/revisioning/git_repository_client.py (Diff revision 8)
Show all issues
```
 'rdb' imported but unused
```
bot/reviewbot/revisioning/repository_client.py (Diff revision 8)
Show all issues
```
Col: 9
 E265 block comment should start with '# '
```
bot/reviewbot/revisioning/repository_clone.py (Diff revision 8)
Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
bot/reviewbot/revisioning/repository_clone.py (Diff revision 8)
Show all issues
```
 undefined name 'self'
```
bot/reviewbot/revisioning/repository_manager.py (Diff revision 8)
Show all issues
```
Col: 29
 E126 continuation line over-indented for hanging indent
```
bot/reviewbot/revisioning/repository_manager.py (Diff revision 8)
Show all issues
```
Col: 17
 E128 continuation line under-indented for visual indent
```

Change Summary:

Finished adding the ability to apply a patch to a directory and have moved where the RepositoryClone object is accessible from.

Description:

		Setting up the bot to handle Git repositories and pass the file paths to the tools.
		Currently in:
		- sets up repository on load
		- supports Git, but easy to extend for new types of repositories
		- stores repository in root
		- determining information about base commit
		- switch to base commit
		- cached path property
		- fixed bug in make_tempfile
		- delete temporary folders
		- update local repository programatically
-		WIP:
		- sending file path to tools
~		- use SCM tool to apply patch to repository
	~	- use SCM tool to apply patch to repository
	+	WIP:
	+	- comment on a file from its path

Branch:

+	master

Commit:

-	f0e81766a7717b6d90b51739d27ea39d737f56e2
+	7d94a4ab845d32d27d8457760ccb323587545da1

Diff:

Revision 9 (+353 -61)

Show changes

	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/processing/filesystem.py
	bot/reviewbot/processing/review.py
	bot/reviewbot/revisioning/__init__.py
	bot/reviewbot/revisioning/git_repository_client.py
	bot/reviewbot/revisioning/repository_client.py
	bot/reviewbot/revisioning/repository_clone.py
	4 more

Tool: PEP8 Style Checker
Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

bot/reviewbot/revisioning/repository_client.py (Diff revision 9)
Show all issues
```
Col: 80
 E501 line too long (85 > 79 characters)
```
bot/reviewbot/revisioning/repository_client.py (Diff revision 9)
Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```
bot/reviewbot/revisioning/repository_manager.py (Diff revision 9)
Show all issues
```
Col: 21
 E128 continuation line under-indented for visual indent
```
bot/reviewbot/tools/__init__.py (Diff revision 9)
Show all issues
```
Col: 19
 E262 inline comment should start with '# '
```

Tool: Pyflakes
Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

bot/reviewbot/processing/filesystem.py (Diff revision 9)
Show all issues
```
 undefined name 'path'
```
bot/reviewbot/processing/review.py (Diff revision 9)
Show all issues
```
 undefined name 'payload'
```
bot/reviewbot/processing/review.py (Diff revision 9)
Show all issues
```
 undefined name 'logger'
```
bot/reviewbot/processing/review.py (Diff revision 9)
Show all issues
```
 undefined name 'repository_manager'
```
bot/reviewbot/processing/review.py (Diff revision 9)
Show all issues
```
 undefined name 'payload'
```
bot/reviewbot/processing/review.py (Diff revision 9)
Show all issues
```
 undefined name 'RepositoryClone'
```
bot/reviewbot/processing/review.py (Diff revision 9)
Show all issues
```
 undefined name 'logger'
```
bot/reviewbot/processing/review.py (Diff revision 9)
Show all issues
```
 undefined name 'repository_clone'
```
bot/reviewbot/tasks.py (Diff revision 9)
Show all issues
```
 'RepositoryClone' imported but unused
```

Description:

~		Setting up the bot to handle Git repositories and pass the file paths to the tools.
~		Currently in:
~		- sets up repository on load
~		- supports Git, but easy to extend for new types of repositories
~		- stores repository in root
~		- determining information about base commit
~		- switch to base commit
~		- cached path property
~		- fixed bug in make_tempfile
~		- delete temporary folders
~		- update local repository programatically
~		- sending file path to tools
~		- use SCM tool to apply patch to repository
~		WIP:
~		- comment on a file from its path
	~	Setting up the bot to handle Git repositories and pass the repository paths to the tools.
	~
	~	Currently in:
	~
	~	Sets up repository on ReviewBot load
	~	- stores repositories in root folder
	~	- updates repositories programatically
	~
	~	Supports Git, but easy to extend for new types of repositories
	~	- switch to base commit
	~	- use SCM tool to apply patch to temp repository
	~
	~	Fixed bug in make_tempfile
	~
	~	Delete temporary folders
	+
	+	Added cached_property to ReviewBot
	+	- turned File properties into cached properties
	+	- cached path property on Repository_Clone
	+
	+	Added new Tool base class to support access to entire repository
	+	- sending repository path to tool
	+	- comment on a file by its path
	+	- created example ReviewBot tool to look at entire repository
	+
	+	Added flag to extension model to signal if ReviewBot tool supports entire directory
	+	- added UI in admin panel to show this
	+
	+	WIP:

Branch:

-	master
+	release-0.2.x

Commit:

-	7d94a4ab845d32d27d8457760ccb323587545da1
+	00a5b44457d0e0915600c29426ebb8db34b85338

Diff:

Revision 10 (+520 -55)

Show changes

	bot/setup.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/utils.py
	bot/reviewbot/processing/filesystem.py
	bot/reviewbot/processing/review.py
	bot/reviewbot/revisioning/__init__.py
	bot/reviewbot/revisioning/git_repository_client.py
	10 more

Tool: PEP8 Style Checker
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/tools/pep8repo.py
    extension/reviewbotext/admin.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

Tool: Pyflakes
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/tools/pep8repo.py
    extension/reviewbotext/admin.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

Change Summary:

Re-added GitPython into setup.py. It must have gotten lost in a rebase.

Commit:

-	00a5b44457d0e0915600c29426ebb8db34b85338
+	cec345787c84793583f8056344c65cea760a4990

Diff:

Revision 11 (+520 -55)

Show changes

	bot/setup.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/utils.py
	bot/reviewbot/processing/filesystem.py
	bot/reviewbot/processing/review.py
	bot/reviewbot/revisioning/__init__.py
	bot/reviewbot/revisioning/git_repository_client.py
	10 more

Tool: PEP8 Style Checker
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/tools/pep8repo.py
    extension/reviewbotext/admin.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py



Tool: Pyflakes
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/tools/pep8repo.py
    extension/reviewbotext/admin.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

Commit:

-	cec345787c84793583f8056344c65cea760a4990
+	6de54d8f333cc52675a6b203ae8f71c44e0eade0

Diff:

Revision 12 (+521 -55)

Show changes

	bot/setup.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/utils.py
	bot/reviewbot/processing/filesystem.py
	bot/reviewbot/processing/review.py
	bot/reviewbot/revisioning/__init__.py
	bot/reviewbot/revisioning/git_repository_client.py
	10 more

Tool: Pyflakes
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/tools/pep8repo.py
    extension/reviewbotext/admin.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py



Tool: PEP8 Style Checker
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/tools/pep8repo.py
    extension/reviewbotext/admin.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

Change Summary:

Added all the tests I check. I also added more queues to Celery so that a bot only takes on tasks for repositories it can handle.

Summary:

-	[WIP] Added support to reviewbot to pull git repositories.
+	Setting up the bot to handle Git repositories and pass the repository paths to the tools

Description:

-		Setting up the bot to handle Git repositories and pass the repository paths to the tools.
-
-		Currently in:
-
		Sets up repository on ReviewBot load
		- stores repositories in root folder
		- updates repositories programatically

		Supports Git, but easy to extend for new types of repositories
		- switch to base commit
		- use SCM tool to apply patch to temp repository

		Fixed bug in make_tempfile

		Delete temporary folders

		Added cached_property to ReviewBot
		- turned File properties into cached properties
		- cached path property on Repository_Clone

		Added new Tool base class to support access to entire repository
		- sending repository path to tool
		- comment on a file by its path
~		- created example ReviewBot tool to look at entire repository
	~	- created example ReviewBot tool to look at entire repository
	+	- added more Queues to Celery so that a bot only picks up a task that it has the repository for

		Added flag to extension model to signal if ReviewBot tool supports entire directory
		- added UI in admin panel to show this
-
-		WIP:

Testing Done:

	+	The following configuration was used in reviewbot/config.py
	+
	+	`bot_repository_settings = {`
	+	"http://localhost:8080/": {
	+	"ReviewBot": {
	+	"type": GIT,
	+	"url": "git://github.com/reviewboard/ReviewBot.git",
	+	},
	+	}
	+
	+
	+
	+
	+	The following tool was added at the path reviewbot/tools/pep8repo.py
	+
	+	`import os`
	+
	+	from reviewbot.tools import RepositoryTool
	+	from reviewbot.tools.process import execute
	+	from reviewbot.utils import is_exe_in_path
	+
	+
	+	class PEP8ToolRepo(RepositoryTool):
	+	name = 'PEP8 Style Checker - Entire Repository'
	+	version = '0.2'
	+	description = "Checks code for style errors using the PEP8 tool."
	+
	+	def check_dependencies(self):
	+	return is_exe_in_path('pep8')
	+
	+	def handle_repository(self, repository_clone):
	+	path = repository_clone.path
	+	if not path:
	+	return False
	+
	+	for root, subFolders, files in os.walk(path):
	+	for file_path in files:
	+	if not file_path.endswith('.py'):
	+	continue
	+
	+	full_path = os.path.join(root, file_path)
	+
	+	output = execute(
	+	[
	+	'pep8',
	+	'-r',
	+	full_path
	+	],
	+	split_lines=True,
	+	ignore_errors=True)
	+
	+	for line in output:
	+	parsed = line.split(':', 3)
	+	lnum = int(parsed[1])
	+	col = int(parsed[2])
	+	msg = parsed[3]
	+	repository_clone.comment(
	+	full_path,
	+	'Col: %s\n%s' % (col, msg),
	+	lnum)
	+
	+	return True
	+
	+
	+
	+
	+	I used a server running on "http://localhost:8080" to test my changes. The setup process is the same as if you were setting up ReviewBot normally. When refreshing the list of tools, PEP8 Style Checker - Entire Repository should show up with a value of true in the 'working directory required' column. To test I set both it and the normal PEP8 Style Checker to run automatically to compare output.
	+
	+	Tests:
	+	1. On the second startup of the bot, it should take less time than the second to start Celery (if you include the reviewboard repository it is more noticeable). This indicates that the repository is being stored between bot sessions as expected. These repositories should stored in a folder system ~/ReviewBotRepositories/server_url/repo_name/.
	+	2. In the celery startup output, it should be shown that one of the Celery Queues the bot is listening on is: pep8repo.0.2.http://localhost:8080/.ReviewBot exchange=pep8repo.0.2.http://localhost:8080/.ReviewBot(direct) key=pep8repo.0.2.http://localhost:8080/.ReviewBot
	+	3. The reviewboard page /admin/extensions/reviewbotext.extension.ReviewBotExtension/db/reviewbotext/reviewbottool/ should show a column called Working directory required. For the tool 'PEP8 Style Checker - Entire Repository' there should be a green check.
	+	5. In the middle of a review being made (celery.contrib.rdb.set_trace() was used to pause in the middle), there should be a file in /tmp/ that contains the patch to apply to the repository to get the code as it is in the review request.
	+	4. In the middle of a review being made (celery.contrib.rdb.set_trace() was used to pause in the middle), there should be a folder in /tmp/ that contains the ReviewBot repository without the .git folder. The state of the repository should be identical to that of the review request.
	+	5. After a publishing a review, checking the /tmp/ folder of the bot should show there are no temp files or directories.
	+	6. Both the normal PEP8 and the full repository PEP8 should complete without error. The resulting code comments should be identical. Of particular note, there should be no comments on files not changed in the review request.

Commit:

-	6de54d8f333cc52675a6b203ae8f71c44e0eade0
+	c5d7445ff571ca0f3ab46e143a67088d0adc28f7

Diff:

Revision 13 (+470 -62)

Show changes

	bot/setup.py
	bot/reviewbot/celery.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/utils.py
	bot/reviewbot/processing/filesystem.py
	bot/reviewbot/processing/review.py
	bot/reviewbot/revisioning/__init__.py
	10 more

Tool: Pyflakes
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/admin.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py



Tool: PEP8 Style Checker
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/admin.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

bot/reviewbot/config.py (Diff revision 13)
Show all issues
```
 'GIT' imported but unused
```

bot/reviewbot/config.py (Diff revision 13)

Show all issues

Maybe I should include this in the comments rather than in the actual code. I really want whoever writes the config to use those constants, hence the inclusion here.

Change Summary:

Fixed bracket issues in the config.py and moved the import into the comments as it isn't actually used.

Commit:

-	c5d7445ff571ca0f3ab46e143a67088d0adc28f7
+	7df8bde15e6f6d74bc636b47dd4a31239591ed39

Diff:

Revision 14 (+471 -62)

Show changes

	bot/setup.py
	bot/reviewbot/celery.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/utils.py
	bot/reviewbot/processing/filesystem.py
	bot/reviewbot/processing/review.py
	bot/reviewbot/revisioning/__init__.py
	10 more

Tool: PEP8 Style Checker
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/admin.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py



Tool: Pyflakes
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/admin.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

bot/reviewbot/celery.py (Diff revision 14)

Show all issues

Capitalize the first word + period (for consistency?)

bot/reviewbot/revisioning/repository_manager.py (Diff revision 14)

Show all issues

You will need a space after been. Otherwise when the Exception is raised it'll be concatenated like:

Respository _ in server __ has not beeninitialized in ...

bot/reviewbot/utils.py (Diff revision 14)
Show all issues
```
Should this class be uppercase?
```
1. MA
  
  matthewcmaclean Aug. 4, 2014, 10:41 a.m.
  Since it is a property, it shouldn't be uppercase.

Thanks for the detailed description! This is mostly a style review since I haven't been following your project closely. I'll try to follow up with another review of the logic though, after I read your Hackpad.

bot/reviewbot/celery.py (Diff revision 14)

Show all issues

Can you remove the extra space and add a period at the end of the sentence?

bot/reviewbot/config.py (Diff revision 14)
Show all issues
```
s/supoort/support
```
bot/reviewbot/config.py (Diff revision 14)
Show all issues
```
s/an/An
```

bot/reviewbot/config.py (Diff revision 14)

Show all issues

Can you capitalize URL? Here and in all your other comments/docstrings.

bot/reviewbot/config.py (Diff revision 14)
Show all issues
```
Remove the apostrophe on "name's".
```

bot/reviewbot/processing/filesystem.py (Diff revision 14)

Show all issues

We generally add full-package imports before single-item imports, so this should go after import tempfile.

bot/reviewbot/processing/filesystem.py (Diff revision 14)

Show all issues

Remove the extra space before and after the sentence, and add a period.

bot/reviewbot/processing/filesystem.py (Diff revision 14)

Show all issues

Can you add a blank line after this? We generally have one blank line before and after if/for/while blocks.

bot/reviewbot/processing/filesystem.py (Diff revision 14)
Show all issues
```
Can you add a blank line before this, while you're here?
```
bot/reviewbot/processing/filesystem.py (Diff revision 14)
Show all issues
```
Add a blank line before this.
```
bot/reviewbot/processing/filesystem.py (Diff revision 14)
Show all issues
```
Remove the extra space before the sentence.
```
bot/reviewbot/processing/filesystem.py (Diff revision 14)
Show all issues
```
Remove the extra space before and after the sentence.
```

bot/reviewbot/processing/review.py (Diff revision 14)

Show all issues

Remove the extra space before and after the sentence. Here and in all other docstrings below.

bot/reviewbot/processing/review.py (Diff revision 14)

Show all issues

Remove the extra space before the sentence, and add a period.

bot/reviewbot/revisioning/git_repository_client.py (Diff revision 14)

Show all issues

This should come directly after from git.exc import NoSuchPathError (since RBTools would be considered as a third-party library import for Review Bot).

bot/reviewbot/revisioning/repository_client.py (Diff revision 14)

Show all issues

This should be:

from os import chdir, get cwd
from re import sub

from django.utils.functional import cached_property
from rbtools.commands.patch import Patch

from reviewbot.processing.filesystem import make_tempfile

bot/reviewbot/revisioning/repository_manager.py (Diff revision 14)
Show all issues
```
We generally use single quotes for strings. (Here and in other places.)
```

bot/reviewbot/revisioning/repository_manager.py (Diff revision 14)

Show all issues

Can you change this to: 
logging.warning(
    'server: %s, repository %s: type%s is not supported' %
    (server_url, repo_name, repo_settings['type']))

bot/reviewbot/revisioning/repository_manager.py (Diff revision 14)

Show all issues

Can you make this into a single-line docstring, like """Returns the server URL and names of repositories the bot supports."""?

bot/reviewbot/tools/__init__.py (Diff revision 14)
Show all issues
```
s/seperately/separately
```

bot/reviewbot/utils.py (Diff revision 14)

Show all issues

Can you remove the extra space before the sentence, and add a period?

bot/reviewbot/utils.py (Diff revision 14)
Show all issues
```
Add a blank line after this.
```

bot/setup.py (Diff revision 14)

Show all issues

Since you removed your pep8repo tool used for testing, this line should also be deleted.

Testing Done:

		The following configuration was used in reviewbot/config.py

		`bot_repository_settings = {`
~		"http://localhost:8080/": {
~		"ReviewBot": {
~		"type": GIT,
~		"url": "git://github.com/reviewboard/ReviewBot.git",
	~	'http://localhost:8080/': {
	~	'ReviewBot': {
	~	'type': GIT,
	~	'url': 'git://github.com/reviewboard/ReviewBot.git',
		},
~		}
	~	},
	+	}




		The following tool was added at the path reviewbot/tools/pep8repo.py

		`import os`

		from reviewbot.tools import RepositoryTool
		from reviewbot.tools.process import execute
		from reviewbot.utils import is_exe_in_path


~		class PEP8ToolRepo(RepositoryTool):
	~	class PEP8RepoTool(RepositoryTool):
		name = 'PEP8 Style Checker - Entire Repository'
		version = '0.2'
		description = "Checks code for style errors using the PEP8 tool."

		def check_dependencies(self):
		return is_exe_in_path('pep8')

		def handle_repository(self, repository_clone):
		path = repository_clone.path
		if not path:
		return False

		for root, subFolders, files in os.walk(path):
		for file_path in files:
		if not file_path.endswith('.py'):
		continue

		full_path = os.path.join(root, file_path)

		output = execute(
		[
		'pep8',
		'-r',
		full_path
		],
		split_lines=True,
		ignore_errors=True)

		for line in output:
		parsed = line.split(':', 3)
		lnum = int(parsed[1])
		col = int(parsed[2])
		msg = parsed[3]
		repository_clone.comment(
		full_path,
		'Col: %s\n%s' % (col, msg),
		lnum)

		return True




		I used a server running on "http://localhost:8080" to test my changes. The setup process is the same as if you were setting up ReviewBot normally. When refreshing the list of tools, PEP8 Style Checker - Entire Repository should show up with a value of true in the 'working directory required' column. To test I set both it and the normal PEP8 Style Checker to run automatically to compare output.

		Tests:
		1. On the second startup of the bot, it should take less time than the second to start Celery (if you include the reviewboard repository it is more noticeable). This indicates that the repository is being stored between bot sessions as expected. These repositories should stored in a folder system ~/ReviewBotRepositories/server_url/repo_name/.
		2. In the celery startup output, it should be shown that one of the Celery Queues the bot is listening on is: pep8repo.0.2.http://localhost:8080/.ReviewBot exchange=pep8repo.0.2.http://localhost:8080/.ReviewBot(direct) key=pep8repo.0.2.http://localhost:8080/.ReviewBot
		3. The reviewboard page /admin/extensions/reviewbotext.extension.ReviewBotExtension/db/reviewbotext/reviewbottool/ should show a column called Working directory required. For the tool 'PEP8 Style Checker - Entire Repository' there should be a green check.
		5. In the middle of a review being made (celery.contrib.rdb.set_trace() was used to pause in the middle), there should be a file in /tmp/ that contains the patch to apply to the repository to get the code as it is in the review request.
		4. In the middle of a review being made (celery.contrib.rdb.set_trace() was used to pause in the middle), there should be a folder in /tmp/ that contains the ReviewBot repository without the .git folder. The state of the repository should be identical to that of the review request.
		5. After a publishing a review, checking the /tmp/ folder of the bot should show there are no temp files or directories.
		6. Both the normal PEP8 and the full repository PEP8 should complete without error. The resulting code comments should be identical. Of particular note, there should be no comments on files not changed in the review request.

Commit:

-	7df8bde15e6f6d74bc636b47dd4a31239591ed39
+	477c77eab9a705bdf5982ccf66ff0d680044cacc

Diff:

Revision 15 (+518 -62)

Show changes

	bot/setup.py
	bot/reviewbot/celery.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/utils.py
	bot/reviewbot/processing/filesystem.py
	bot/reviewbot/processing/review.py
	bot/reviewbot/revisioning/__init__.py
	11 more

Tool: PEP8 Style Checker
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/tools/pep8repo.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/admin.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py



Tool: Pyflakes
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/tools/pep8repo.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/admin.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

Change Summary:

Removed pep8repo.py which I by mistake added back in.

Commit:

-	477c77eab9a705bdf5982ccf66ff0d680044cacc
+	c246791217e90f8078d1139de7d74cea732a04b9

Diff:

Revision 16 (+471 -62)

Show changes

	bot/setup.py
	bot/reviewbot/celery.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/utils.py
	bot/reviewbot/processing/filesystem.py
	bot/reviewbot/processing/review.py
	bot/reviewbot/revisioning/__init__.py
	10 more

Tool: Pyflakes
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/admin.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

Tool: PEP8 Style Checker
Processed Files:
    bot/setup.py
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/admin.py
    bot/reviewbot/revisioning/repository_clone.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

This looks great! You may not have time for this, but I think it'd be good to also grab the branch information off the review request, and actually checkout that branch before switching to the base commit and applying the patch.

bot/reviewbot/celery.py (Diff revision 16)

Show all issues

If there are multiple tools with working_directory_required == True, this will initialize the RepositoryManager multiple times. Each queue would be added several times extra too.

Also, I'm just curious if there is a reason for having separate queues for each repository (instead of a single pep8repo tool queue, for example)?

smacleod Aug. 6, 2014, 8:35 a.m.

The reason for each repository to have it's own queue is so particular workers can be configured to only service a subset of the repositories. In some organizations, repositories could be quite large, and there could be many of them - this multiple queue system allows specific workers to handle only certain repos.

anselina Aug. 6, 2014, 9:08 a.m.
```
Cool, that makes sense!
```

bot/reviewbot/revisioning/git_repository_client.py (Diff revision 16)

Show all issues

If the patch doesn't apply cleanly or raises an error, we probably shouldn't proceed with the review. execute() has some parameters dealing with errors.

matthewcmaclean Aug. 9, 2014, 3:44 p.m.

So the current behaviour is that if there is an error in executing the command, it will be thrown in execute via a call to sys.exit(1). This will terminate the process running the review on the repository; however, the bot will remain up. So any subsequent calls to the bot will be handled as normal. The error thrown will depend on what caused the command to fail, but an example error is:

[2014-08-09 15:00:13,520: WARNING/Worker-1] Failed to execute command: ['git', 'apply', '/tmp/tmpsrBBQ9']
error: patch failed: bot/reviewbot/celery.py:6
error: bot/reviewbot/celery.py: patch does not apply
error: bot/reviewbot/config.py: already exists in working directory
error: patch failed: bot/reviewbot/processing/filesystem.py:1
error: bot/reviewbot/processing/filesystem.py: patch does not apply
error: patch failed: bot/reviewbot/processing/review.py:1
error: bot/reviewbot/processing/review.py: patch does not apply
error: bot/reviewbot/revisioning/init.py: already exists in working directory
error: bot/reviewbot/revisioning/git_repository_client.py: already exists in working directory
error: bot/reviewbot/revisioning/repository_client.py: already exists in working directory
error: bot/reviewbot/revisioning/repository_clone.py: already exists in working directory
error: bot/reviewbot/revisioning/repository_manager.py: already exists in working directory
error: patch failed: bot/reviewbot/tasks.py:56
error: bot/reviewbot/tasks.py: patch does not apply
error: patch failed: bot/reviewbot/tools/init.py:1
error: bot/reviewbot/tools/init.py: patch does not apply
error: patch failed: bot/reviewbot/utils.py:20
error: bot/reviewbot/utils.py: patch does not apply
error: patch failed: bot/setup.py:28
error: bot/setup.py: patch does not apply
error: patch failed: extension/reviewbotext/admin.py:17
error: extension/reviewbotext/admin.py: patch does not apply
error: patch failed: extension/reviewbotext/extension.py:66
error: extension/reviewbotext/extension.py: patch does not apply
error: patch failed: extension/reviewbotext/handlers.py:23
error: extension/reviewbotext/handlers.py: patch does not apply
error: patch failed: extension/reviewbotext/models.py:24
error: extension/reviewbotext/models.py: patch does not apply
error: patch failed: extension/reviewbotext/resources.py:170
error: extension/reviewbotext/resources.py: patch does not apply
[2014-08-09 15:00:14,529: ERROR/MainProcess] Process 'Worker-1' pid:9312 exited with 'exitcode 1'
[2014-08-09 15:00:14,541: ERROR/MainProcess] Task reviewbot.tasks.ProcessReviewRequest[13c1bc14-53eb-435c-8a75-b7d75b721da2] raised unexpected: WorkerLostError('W
orker exited prematurely: exitcode 1.',)
Traceback (most recent call last):
File "/home/vagrant/rbenv/local/lib/python2.7/site-packages/billiard-3.3.0.17-py2.7-linux-x86_64.egg/billiard/pool.py", line 1169, in mark_as_worker_lost
human_status(exitcode)),
WorkerLostError: Worker exited prematurely: exitcode 1.

The alternative to this is using the 'ignore_errors' flag in execute and instead using the 'none_on_ignored_error' flag. Then I could catch the result in the apply patch command and throw a custom Exception, but this would by-pass https://github.com/reviewboard/rbtools/blob/master/rbtools/utils/process.py#L7, which to be fair doesn't do a whole lot at the moment.

Do you have a preference on how to handle the error?

anselina Aug. 9, 2014, 8:44 p.m.

Sorry, should've been more specific! The latter is what I had in mind (setting ignore_errors=True and none_on_ignored_error=True and checking if execute() returns None) since in the new API, workers can update tool executions with their status and result. So in this case, the worker would update the tool execution as being "failed" and put the error message (e.g., "Patch did not apply cleanly") as the result.

matthewcmaclean Aug. 9, 2014, 10:15 p.m.

Cool, so I check for the None return in the git_repository_client.py. The other option is _apply_patch_to_current_directory could return None if it fails and some other value if it succeeds and then repository_client.py could throw the exception for all SCM implementations.
Since we only care if it is an error, the more appropriate manner to communicate that seems to be through exception rather than return value. As well, we don't know the return values of future SCM patch implementations, so it seemed better to throw the exception in git_repository_client.py rather than in repository_client.py.

bot/reviewbot/revisioning/repository_clone.py (Diff revision 16)

Show all issues

Can we call this RepositoryWorkingDirectory instead (or something to that effect), and change the docstring to "Represents a working directory of a specified repository."? It's easy to mistake this to be an actual clone of the repo.

bot/reviewbot/revisioning/repository_manager.py (Diff revision 16)

Show all issues

Can we put this in a function like initialize_repository_manager(), and just add a call to this in celery.py if a working directory is required for one of the tools? This will make it more testable for the future.

matthewcmaclean Aug. 9, 2014, 3:44 p.m.

Would we want to move all the contents of this file into init.py in revisioning? The file was originally made since there was a manager object. I don't know what best practice is for that.

anselina Aug. 9, 2014, 8:44 p.m.

I think it's a good idea to move this into revisioning/__init__.py.

matthewcmaclean Aug. 9, 2014, 10:15 p.m.

So I moved the repository_manager.py stuff into revisioning/__init__.py; however, it resulted in a circular dependency between config.py and revisioning/__init__.py. So I moved the declaration of GIT and REPOSITORY_MAP to revisioning/constants.py. This helps localize changes to constants when you add a new SCM and it means that revisioning/__init__.py doesn't need to import GIT anymore.
I also switched the other global variables to be all caps.

bot/reviewbot/tasks.py (Diff revision 16)
Show all issues
```
Can you indent this an extra level?
```

bot/reviewbot/tools/__init__.py (Diff revision 16)

Show all issues

"static" should be removed since we can now do more than just static analysis with the full working directory.

extension/reviewbotext/resources.py (Diff revision 16)
Show all issues
```
Indent this an extra level.
```

Change Summary:

Updated code as per the review. Not all the issues have been resolved as I have further questions on the implementation.

Testing Done:

		The following configuration was used in reviewbot/config.py

~		`bot_repository_settings = {`
	~	`from reviewbot.revisioning import GIT`
	+
	+
	+	bot_repository_settings = {
		'http://localhost:8080/': {
		'ReviewBot': {
		'type': GIT,
		'url': 'git://github.com/reviewboard/ReviewBot.git',
		},
		},
		}




		The following tool was added at the path reviewbot/tools/pep8repo.py

		`import os`

		from reviewbot.tools import RepositoryTool
		from reviewbot.tools.process import execute
		from reviewbot.utils import is_exe_in_path


		class PEP8RepoTool(RepositoryTool):
		name = 'PEP8 Style Checker - Entire Repository'
		version = '0.2'
		description = "Checks code for style errors using the PEP8 tool."

		def check_dependencies(self):
		return is_exe_in_path('pep8')

~		def handle_repository(self, repository_clone):
~		path = repository_clone.path
	~	def handle_repository(self, repository_working_directory):
	~	path = repository_working_directory.path
		if not path:
		return False

		for root, subFolders, files in os.walk(path):
		for file_path in files:
		if not file_path.endswith('.py'):
		continue

		full_path = os.path.join(root, file_path)

		output = execute(
		[
		'pep8',
		'-r',
		full_path
		],
		split_lines=True,
		ignore_errors=True)

		for line in output:
		parsed = line.split(':', 3)
		lnum = int(parsed[1])
		col = int(parsed[2])
		msg = parsed[3]
~		repository_clone.comment(
	~	repository_working_directory.comment(
		full_path,
		'Col: %s\n%s' % (col, msg),
		lnum)

		return True




		I used a server running on "http://localhost:8080" to test my changes. The setup process is the same as if you were setting up ReviewBot normally. When refreshing the list of tools, PEP8 Style Checker - Entire Repository should show up with a value of true in the 'working directory required' column. To test I set both it and the normal PEP8 Style Checker to run automatically to compare output.

		Tests:
		1. On the second startup of the bot, it should take less time than the second to start Celery (if you include the reviewboard repository it is more noticeable). This indicates that the repository is being stored between bot sessions as expected. These repositories should stored in a folder system ~/ReviewBotRepositories/server_url/repo_name/.
		2. In the celery startup output, it should be shown that one of the Celery Queues the bot is listening on is: pep8repo.0.2.http://localhost:8080/.ReviewBot exchange=pep8repo.0.2.http://localhost:8080/.ReviewBot(direct) key=pep8repo.0.2.http://localhost:8080/.ReviewBot
		3. The reviewboard page /admin/extensions/reviewbotext.extension.ReviewBotExtension/db/reviewbotext/reviewbottool/ should show a column called Working directory required. For the tool 'PEP8 Style Checker - Entire Repository' there should be a green check.
		5. In the middle of a review being made (celery.contrib.rdb.set_trace() was used to pause in the middle), there should be a file in /tmp/ that contains the patch to apply to the repository to get the code as it is in the review request.
		4. In the middle of a review being made (celery.contrib.rdb.set_trace() was used to pause in the middle), there should be a folder in /tmp/ that contains the ReviewBot repository without the .git folder. The state of the repository should be identical to that of the review request.
		5. After a publishing a review, checking the /tmp/ folder of the bot should show there are no temp files or directories.
		6. Both the normal PEP8 and the full repository PEP8 should complete without error. The resulting code comments should be identical. Of particular note, there should be no comments on files not changed in the review request.

Commit:

-	c246791217e90f8078d1139de7d74cea732a04b9
+	8a19bad9df86e4b5bb32316a0c9c4ed2e14a3153

Diff:

Revision 17 (+485 -62)

Show changes

	bot/setup.py
	bot/reviewbot/celery.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/utils.py
	bot/reviewbot/processing/filesystem.py
	bot/reviewbot/processing/review.py
	bot/reviewbot/revisioning/__init__.py
	10 more

Tool: PEP8 Style Checker
Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_working_dir.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/admin.py
    bot/setup.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

Tool: Pyflakes
Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/repository_working_dir.py
    bot/reviewbot/revisioning/repository_manager.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/admin.py
    bot/setup.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

bot/reviewbot/celery.py (Diff revision 17)

Show all issues

Actually, sorry, instead of calling initialize_repositories() here, how about we call it in supported_repositories()?

bot/reviewbot/revisioning/repository_manager.py (Diff revision 17)
Show all issues
```
Globals are usually capitalized, so can you make this be REPOSITORIES?
```

bot/reviewbot/revisioning/repository_manager.py (Diff revision 17)

Show all issues

Let's set REPOSITORIES = None in line 12, and set REPOSITORIES = {} here.
This way, we can just do if REPOSITORIES is None to check if the RepositoryClients have been instantiated/initialized, so we don't need the initialized boolean anymore.
Let's actually also move this logic to supported_repositories(), so it'll look like:
def supported_repositories():
    global REPOSITORIES

    if REPOSITORIES is None:
        initialize_repositories()

    return REPOSITORIES.keys()

anselina Aug. 9, 2014, 9:14 p.m.

Oops, forgot one more thing - we won't need the global REPOSITORIES line in supported_repositories() or get_repository(). It's only needed to modify/create a global variable, not to access one.

Testing Done:

		The following configuration was used in reviewbot/config.py

~		`from reviewbot.revisioning import GIT`
	~	`from reviewbot.revisioning.constants import GIT`


~		bot_repository_settings = {
	~	BOT_REPOSITORY_SETTINGS = {
		'http://localhost:8080/': {
		'ReviewBot': {
		'type': GIT,
		'url': 'git://github.com/reviewboard/ReviewBot.git',
		},
		},
		}




		The following tool was added at the path reviewbot/tools/pep8repo.py

		`import os`

		from reviewbot.tools import RepositoryTool
		from reviewbot.tools.process import execute
		from reviewbot.utils import is_exe_in_path


		class PEP8RepoTool(RepositoryTool):
		name = 'PEP8 Style Checker - Entire Repository'
		version = '0.2'
		description = "Checks code for style errors using the PEP8 tool."

		def check_dependencies(self):
		return is_exe_in_path('pep8')

		def handle_repository(self, repository_working_directory):
		path = repository_working_directory.path
		if not path:
		return False

		for root, subFolders, files in os.walk(path):
		for file_path in files:
		if not file_path.endswith('.py'):
		continue

		full_path = os.path.join(root, file_path)

		output = execute(
		[
		'pep8',
		'-r',
		full_path
		],
		split_lines=True,
		ignore_errors=True)

		for line in output:
		parsed = line.split(':', 3)
		lnum = int(parsed[1])
		col = int(parsed[2])
		msg = parsed[3]
		repository_working_directory.comment(
		full_path,
		'Col: %s\n%s' % (col, msg),
		lnum)

		return True




		I used a server running on "http://localhost:8080" to test my changes. The setup process is the same as if you were setting up ReviewBot normally. When refreshing the list of tools, PEP8 Style Checker - Entire Repository should show up with a value of true in the 'working directory required' column. To test I set both it and the normal PEP8 Style Checker to run automatically to compare output.

		Tests:
		1. On the second startup of the bot, it should take less time than the second to start Celery (if you include the reviewboard repository it is more noticeable). This indicates that the repository is being stored between bot sessions as expected. These repositories should stored in a folder system ~/ReviewBotRepositories/server_url/repo_name/.
		2. In the celery startup output, it should be shown that one of the Celery Queues the bot is listening on is: pep8repo.0.2.http://localhost:8080/.ReviewBot exchange=pep8repo.0.2.http://localhost:8080/.ReviewBot(direct) key=pep8repo.0.2.http://localhost:8080/.ReviewBot
		3. The reviewboard page /admin/extensions/reviewbotext.extension.ReviewBotExtension/db/reviewbotext/reviewbottool/ should show a column called Working directory required. For the tool 'PEP8 Style Checker - Entire Repository' there should be a green check.
		5. In the middle of a review being made (celery.contrib.rdb.set_trace() was used to pause in the middle), there should be a file in /tmp/ that contains the patch to apply to the repository to get the code as it is in the review request.
		4. In the middle of a review being made (celery.contrib.rdb.set_trace() was used to pause in the middle), there should be a folder in /tmp/ that contains the ReviewBot repository without the .git folder. The state of the repository should be identical to that of the review request.
		5. After a publishing a review, checking the /tmp/ folder of the bot should show there are no temp files or directories.
		6. Both the normal PEP8 and the full repository PEP8 should complete without error. The resulting code comments should be identical. Of particular note, there should be no comments on files not changed in the review request.

Commit:

-	8a19bad9df86e4b5bb32316a0c9c4ed2e14a3153
+	d8503aa8a33025000bfcc70b6acd3deb48eaca66

Diff:

Revision 18 (+492 -62)

Show changes

	bot/setup.py
	bot/reviewbot/celery.py
	bot/reviewbot/config.py
	bot/reviewbot/tasks.py
	bot/reviewbot/utils.py
	bot/reviewbot/processing/filesystem.py
	bot/reviewbot/processing/review.py
	bot/reviewbot/revisioning/__init__.py
	10 more

Tool: PEP8 Style Checker
Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/admin.py
    bot/setup.py
    bot/reviewbot/revisioning/repository_working_dir.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

Tool: Pyflakes
Processed Files:
    bot/reviewbot/tasks.py
    bot/reviewbot/revisioning/git_repository_client.py
    bot/reviewbot/tools/__init__.py
    bot/reviewbot/revisioning/constants.py
    bot/reviewbot/processing/filesystem.py
    extension/reviewbotext/admin.py
    bot/setup.py
    bot/reviewbot/revisioning/repository_working_dir.py
    bot/reviewbot/celery.py
    extension/reviewbotext/handlers.py
    extension/reviewbotext/resources.py
    extension/reviewbotext/extension.py
    bot/reviewbot/utils.py
    extension/reviewbotext/models.py
    bot/reviewbot/revisioning/__init__.py
    bot/reviewbot/processing/review.py
    bot/reviewbot/config.py
    bot/reviewbot/revisioning/repository_client.py

Setting up the bot to handle Git repositories and pass the repository paths to the tools

Information

Reviewers

Description:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Description:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Description:

Commit:

Diff:

Groups:

Change Summary:

Description:

Commit:

Diff:

Description:

Commit:

Diff:

Change Summary:

Description:

Branch:

Commit:

Diff:

Description:

Currently in:

WIP:

Branch:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Commit:

Diff:

Change Summary:

Summary:

Description:

Currently in:

WIP:

Testing Done:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Testing Done:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Testing Done:

Commit:

Diff:

Testing Done:

Commit:

Diff:

Status: Discarded