Typo in description: GitHub -> GitLab
You also have what looks like was supposed to be bullet points but isn't, because wrapping happened.

j before l

Missing a format specifier in here for message?

typo: argsuments

Should have a trailing comma

Mind adding args/raises to this docstring?

Mind adding args/returns/raises?

Mind adding args/returns/raises?

Mind adding args/returns/raises?

Mind adding args/returns/raises?

gitlab -> GitLab
Should we use double backticks to be consistent with other documentation?

Mind adding args/returns/raises?

Mind adding returns/raises?

Typo: Retur
Mind adding returns/raises?

Docstring?

Mind adding args/returns/raises?

returns -> return

Otherwise what?

returns -> return

Docstring?

Header keys should be bytestrings

Docstring?

Should we add some extra context to this log message?

Typo in description: "Newer version" -> "Newer versions"

This should likely be __repr__

Still missing real docstring content here.

typo: succesfully -> successfully

Too many blank lines.

HTTPError is a subclass of URLError, and URLError comes from urllib2.

HTTPError is a subclass of URLError, and URLError comes from urllib2.

HTTPError is a subclass of URLError, and URLError comes from urllib2.

HTTPError is a subclass of URLError, and URLError comes from urllib2.

HTTPError is a subclass of URLError, and URLError comes from urllib2.

HTTPError is a subclass of URLError, and URLError comes from urllib2.

HTTPError is a subclass of URLError, and URLError comes from urllib2.

HTTPError is a subclass of URLError, and URLError comes from urllib2.

HTTPError is a subclass of URLError, and URLError comes from urllib2.

typo: __repr_ -> __repr__

Should be two lines.

Typo: Return -> Returns

Needs Returns

This seems like a bad log message.

There's an extra space after "Personal Access" that shouldn't be there.

These two lines could be one.

Gitlab -> GitLab

typo: acount

How about wrapping this as:

return self._api_get(self.account.hosting_url,
                     'groups/%s' % group_id)[0]

Same here re: wrapping.

Header keys should be binary.

There's an extra comma at the end of this line, and the ) shouldn't be on the next line.

Closing ) from function call shouldn't be on its own line, plus there's an extra comma after the last arg.

Trailing ) for function calls shouldn't be on its own line.

Indentation got messed up here.

Closing ) for function call shouldn't be on its own line. There's also an extra , after the last arg.

Needs a trailing comma.

Needs a trailing comma.

Needs a trailing comma.

Should use double-backticks, since this is rst and not markdown.

GitLab API tests should continue to test API v3, with additional tests for API v4, rather than just moving them all to v4. This ensures we don't regress for users. Might be worth making a test class for each.

Make sure you build the docs and check for new errors. There are lots of syntax and reference errors throughout that would come up.

This should probably be a HostingServiceError.

You should be able to just pass self.causes into the %r directly. It'll repr the list, which will then auto-repr each item in the list.

Help text shouldn't use <br>. One long string is better here.

, unused after the type.

Other docs don't do "Raised if..." That's implied and can be left off.
For the last one, "occurred" would be more correct.

"SHA1", and of the blob for the file.

"SHA1"

, unused

Same comment about the "Raised if ..."

Same notes as above.

Same notes as above about "Raised if ..."

While you're here, this should be "Return a list of branches."

This isn't correct.

Same notes as above.

This isn't correct. Shoud be reviewboard.scmtools.core.Commit.

Same notes as above with "Raised if ..."

reviewboard.scmtools.core.Commit

Same notes as above with "Raised if ..."

Can you include the comment from above here as well, about why we're checking both message and title?

Same notes about "Raised if ..."

I assume it's checking both for GitLab API version differences? Can you document those here?

While here, can you add a blank line between these?

Same notes about "Raised if ..."

Same here.

Same here.

:samp:

Will the old version work on the new API?

Typo: "reviewboard"

Same note about "Raised if ..."

Here too.

Same notes about "Raised if ..."

This is a pretty weird and hard-to-read statement, particularly with all the layers of nots and the ^. Given that it's an internal function, and we're responsible for the arguments, I'm not sure we need an exception (which is also not helpful to users if we do screw up with the call and it bubbles up). It's also probably not a big deal if all three are passed in, in practice. How about just doing this further down:

if url:
    assert not hosting_url
    assert not path
else:
    url = ...

Same comments about "Raised if ..."

"Return the version ..."

Syntax error. Missing the backtick.

I don't see how we'd ever be in a case where the or would occur. If we hit an exception, we won't get a value here. If we get a result at all, it's going to have a value, due to the implementation in _try_api_versions. I want to make sure I'm not missing something.

Typo: "arguments"

Same notes about the "Raised if ..."

This should never happen. If it does, it's an internal failure. We should assert.

You can use basestring.

You can use basestring.

I might be wrong about some part of this, but according urlopen docs, the first argument to urlopen is url, not a request.
Naming of parameters should also be consistent now, due to more strict kgb internals.
Same with all other fake _urlopen functions.

assertIsNone

Can we keep the v3 API unit tests, and just add new v4 tests, instead of replacing them?

This should probably he a HostingServiceError. That way it's properly displayed if it bubbles up.

Mind putting those in alphabetical order?

There's two places where we do this, and have to maintain version compatibility. How about defining a _parse_commit() method that handles this?
I know the other location that does this has a different flow, but I have a thought there. See my next comment.

And here we are.
This method delays building the Commit until the very end, after we've dealt with the initial data and then generated the diff content. In the case where it's cached, we already have a Commit object that we throw away, though. So my thought is this that we can do this:

if commit is None:
    ...

    commit_rsp = self._api_get(...)
    commit = self._parse_commit(commit_rsp)


# Diff generation here...

commit.diff = diff

return commit


Should simplify much of this code.

Blank line between these.

Alphabetical order.

Do we have pagination for groups in the new API

Same question about repositories.

Should probably be bytes

Private method, so this should go after all public methods.

There's some old code still here.