Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/gitlab.py

Col: 47
 E127 continuation line over-indented for visual indent

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

Col: 6
 W291 trailing whitespace

Col: 1
 W293 blank line contains whitespace

Col: 5
 E303 too many blank lines (2)

Col: 1
 W293 blank line contains whitespace

Col: 28
 E241 multiple spaces after ','

Col: 35
 E128 continuation line under-indented for visual indent

Col: 35
 E128 continuation line under-indented for visual indent

Col: 49
 E127 continuation line over-indented for visual indent

Col: 35
 E703 statement ends with a semicolon

Col: 13
 E113 unexpected indentation

Looks like there's still a bit of work to do here. Aside from the comments listed below, this still needs a rewrite of the description, fully-comprehensive unit tests, and information on every real-world test you ran against a live GitLab server.

That's a GitHub-specific thing.

Make sure all trailing whitespace is cleaned up.

If functions aren't being used, we don't want to introduce them yet, even in commented-out form.

None of these functions are doing anything. Having them here is just going to add confusion. Make sure we're only including what's necessary in this change for post-commit review to work.

These are all missing docstrings.

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/github.py
    reviewboard/hostingsvcs/gitlab.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/gitlab.py

Col: 47
 E127 continuation line over-indented for visual indent

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

Col: 6
 W291 trailing whitespace

Col: 1
 W293 blank line contains whitespace

Col: 5
 E303 too many blank lines (2)

Col: 1
 W293 blank line contains whitespace

Col: 72
 W291 trailing whitespace

Col: 28
 E241 multiple spaces after ','

Col: 31
 E127 continuation line over-indented for visual indent

Col: 35
 E128 continuation line under-indented for visual indent

Col: 35
 E128 continuation line under-indented for visual indent

Col: 73
 W291 trailing whitespace

Col: 35
 E703 statement ends with a semicolon

Col: 13
 E113 unexpected indentation

Col: 47
 E127 continuation line over-indented for visual indent

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

Col: 6
 W291 trailing whitespace

Col: 1
 W293 blank line contains whitespace

Col: 72
 W291 trailing whitespace

Col: 28
 E241 multiple spaces after ','

Col: 31
 E127 continuation line over-indented for visual indent

Col: 73
 W291 trailing whitespace

Col: 35
 E703 statement ends with a semicolon

Col: 13
 E113 unexpected indentation

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/gitlab.py

Col: 45
 E128 continuation line under-indented for visual indent

Col: 6
 W291 trailing whitespace

Col: 1
 W293 blank line contains whitespace

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/gitlab.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/gitlab.py

 'cache' imported but unused

Col: 45
 E128 continuation line under-indented for visual indent

Col: 6
 W291 trailing whitespace

Col: 1
 W293 blank line contains whitespace

 undefined name 'HTTPERRor'

 undefined name 'logging'

 undefined name 'SCMError'

 undefined name 'logging'

 undefined name 'logging'

 undefined name 'SCMError'

 undefined name 'logging'

 undefined name 'SCMError'

 local variable 'gitlab_msg' is assigned to but never used

 undefined name 'ref_name'

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/gitlab.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/gitlab.py

 'cache' imported but unused

Col: 45
 E128 continuation line under-indented for visual indent

 undefined name 'HTTPERRor'

 undefined name 'logging'

 undefined name 'SCMError'

 undefined name 'logging'

 undefined name 'logging'

 undefined name 'SCMError'

 undefined name 'logging'

 undefined name 'SCMError'

 local variable 'gitlab_msg' is assigned to but never used

 undefined name 'ref_name'

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/gitlab.py



Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/gitlab.py

 'cache' imported but unused

Col: 45
 E128 continuation line under-indented for visual indent

 local variable 'response_info' is assigned to but never used

A lot of the functions you have in GitLabClient are unneccessary. I'm mostly referring to the http_get(), http_post() ones, but I don't think you need to get the tree either for these APIs to work.

This is a GitHub private function, I don't believe we need to check rate limits for GitLab APIs, at least not to implement post commit APIs.

This is redundant with _api_get() that already exists. This will actually not work because GitLab APIs require authentication, see the PRIVATE-TOKEN in _api_get(). I suggest calling that instead.

Why not just if branch: since you do start = branch right before?

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/gitlab.py

Col: 45
 E128 continuation line under-indented for visual indent

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

Col: 17
 E113 unexpected indentation

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

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

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

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/gitlab.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/gitlab.py

 'urllib2' imported but unused

 local variable 'repo_id' is assigned to but never used

Col: 24
 E127 continuation line over-indented for visual indent

This is looking a lot cleaner! :)

I don't think these comments are needed since the code does a good job of explaining it already.

Can we combine these?
commit_api_url = '%s/repository/commits/%s?private_token=%s' % (repo_api_url, revision, private_token)

Can we also combine these?

Please use %-formatting.

Blank line between these.

This should be combined into one:

repo_api_url = '%s/repository/commits?private_token=%s' % (self._get_repo_api_url(repository), self._get_private_token())

Blank line between these.

No comma on the last line -- it is a function call, not a list/dict

Blank line between these.

This seems like good information for the function docstring.

Blank line between these.

Have you tried this for the case where the current revision has no parents (i.e., it is the initial commit in the repository)?

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/gitlab.py
    reviewboard/hostingsvcs/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/gitlab.py
    reviewboard/hostingsvcs/tests.py

Col: 1
 W293 blank line contains whitespace

Col: 61
 W291 trailing whitespace

Col: 30
 E128 continuation line under-indented for visual indent

Col: 30
 E128 continuation line under-indented for visual indent

Col: 76
 W291 trailing whitespace

Col: 1
 W293 blank line contains whitespace

Col: 17
 E126 continuation line over-indented for hanging indent

Col: 17
 E126 continuation line over-indented for hanging indent

Col: 17
 E126 continuation line over-indented for hanging indent

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/gitlab.py
    reviewboard/hostingsvcs/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/gitlab.py
    reviewboard/hostingsvcs/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/gitlab.py
    reviewboard/hostingsvcs/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/gitlab.py
    reviewboard/hostingsvcs/tests.py

Col: 34
 E226 missing whitespace around arithmetic operator

Tool: Pyflakes
Processed Files:
    reviewboard/hostingsvcs/gitlab.py
    reviewboard/hostingsvcs/tests.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/hostingsvcs/gitlab.py
    reviewboard/hostingsvcs/tests.py

<p>So summary and testing done should be really high level descriptions of the changes in the code, e.g. "Previously it was the case that X. Nowe we have corrected that to Y. We chose Y because Z." It usually shouldn't go into the classes/fields/functions modified.</p>

<p>As an example, you can say something like "The GitLab hosting service is now able to retrieve branches and commits" instead of listing the <code>get_changes</code> and <code>get_branches</code>.</p>

<p>Have a look at <a href="https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/">Writing Good Descriptions</a> in the codebase docs if you need more information or feel free to respond to this comment.</p>

Alphabetize these.

This sentence doesn't really add anything to the docstring.

See my previous comment. Also instead of saying "The start" you should say "The start parameter".

"The branch is optional. If it is not given, use the default branch."

Is item ever not a dict? If it is something else, what is it and why do we ignore it. This seems like a good thing to document with comments.
If not isinstance(item, dict) is indicative of an error, we should log something.

These comments don't really add anything.

I mentioned this in a previous review, but there should be a blank line between these. It is a style consistency thing.

Reword this. You should mention that a long SHA (capitalized) is 40 characters.

emove this line.

Can we use repo_api_url as an argument in the %-formatting expression?

"a diff"

Can you use < and > instead of " and " here?

Can we use a %-formatting expression here?

This seems like an issue prone way to strip the last two lines. Can we do a reverse search from the end of the diff for the newline character and then strip the rest of the string? Or do an rsplit with maxsep=2 and sep='\n' and only keep the first part?

Blank line between these.

This isn't a necessary comment.