-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 1) Col: 47 E127 continuation line over-indented for visual indent
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 1) Col: 5 E265 block comment should start with '# '
-
-
-
-
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 1) Col: 35 E128 continuation line under-indented for visual indent
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 1) Col: 35 E128 continuation line under-indented for visual indent
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 1) Col: 49 E127 continuation line over-indented for visual indent
-
-
Implement post-commit review request support for GitLab
Review Request #6872 — Created Feb. 1, 2015 and submitted
Add post-commit review request support GitLab
In /r/new/, you can manually view or add review requests for changes already being pushed to remote repository, which is called 'post-commit'. But Reviewboard previously does not support GitLab for this functionality. This project is to add the 'post-commit' review request support for GitLab.
With this implementation, users can retrieve GitLab hosting service information to do 'post-commit' review request operations. Specifically, we can get branches, commits belong to the particular branch, and changes have been made in commits from repositories on GitLab.
There are three general steps are involved. First, we send an API request to GitLab then retrieve a list of branches. Second, we send another API request to get a list of commits with specified branch. After that, when the user clicks on one commit from the page, we send a HTTP request instead of using GitLab API url format to get the diff.
The reason we don't use GitLab API request url to retrieve the diff is the returned value is not what we expect and there are no indices in it. The url format we are using is
https://gitlab.com/<user-name>/<project-name>/commit/<revision>.diff
- Tested on local server, ran as expected.
-- Created a test GitLab repository, successfully fetched branches and commits information in /r/new.
-- New page of commits are displayed normally after scrolling down, pagination is working correctly.
-- Commit diffs can be viewed by clicking commits in the GitLab repository. - Unit tests passed.
-- test_get_branches()
-- test_get_commits()
-- test_get_change()
Description | From | Last Updated |
---|---|---|
Col: 47 E127 continuation line over-indented for visual indent |
reviewbot | |
That's a GitHub-specific thing. |
chipx86 | |
Col: 5 E265 block comment should start with '# ' |
reviewbot | |
Col: 6 W291 trailing whitespace |
reviewbot | |
Make sure all trailing whitespace is cleaned up. |
chipx86 | |
If functions aren't being used, we don't want to introduce them yet, even in commented-out form. |
chipx86 | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 5 E303 too many blank lines (2) |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
None of these functions are doing anything. Having them here is just going to add confusion. Make sure we're only … |
chipx86 | |
Col: 28 E241 multiple spaces after ',' |
reviewbot | |
Col: 35 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 35 E128 continuation line under-indented for visual indent |
reviewbot | |
These are all missing docstrings. |
chipx86 | |
Col: 49 E127 continuation line over-indented for visual indent |
reviewbot | |
Col: 35 E703 statement ends with a semicolon |
reviewbot | |
Col: 13 E113 unexpected indentation |
reviewbot | |
Col: 47 E127 continuation line over-indented for visual indent |
reviewbot | |
Col: 5 E265 block comment should start with '# ' |
reviewbot | |
Col: 6 W291 trailing whitespace |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 5 E303 too many blank lines (2) |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 72 W291 trailing whitespace |
reviewbot | |
Col: 28 E241 multiple spaces after ',' |
reviewbot | |
Col: 31 E127 continuation line over-indented for visual indent |
reviewbot | |
Col: 35 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 35 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 73 W291 trailing whitespace |
reviewbot | |
Col: 35 E703 statement ends with a semicolon |
reviewbot | |
Col: 13 E113 unexpected indentation |
reviewbot | |
Col: 47 E127 continuation line over-indented for visual indent |
reviewbot | |
Col: 5 E265 block comment should start with '# ' |
reviewbot | |
Col: 6 W291 trailing whitespace |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 72 W291 trailing whitespace |
reviewbot | |
Col: 28 E241 multiple spaces after ',' |
reviewbot | |
Col: 31 E127 continuation line over-indented for visual indent |
reviewbot | |
Col: 73 W291 trailing whitespace |
reviewbot | |
Col: 35 E703 statement ends with a semicolon |
reviewbot | |
Col: 13 E113 unexpected indentation |
reviewbot | |
Col: 45 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 6 W291 trailing whitespace |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
'cache' imported but unused |
reviewbot | |
Col: 45 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 6 W291 trailing whitespace |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
undefined name 'HTTPERRor' |
reviewbot | |
undefined name 'logging' |
reviewbot | |
undefined name 'SCMError' |
reviewbot | |
undefined name 'logging' |
reviewbot | |
undefined name 'logging' |
reviewbot | |
undefined name 'SCMError' |
reviewbot | |
undefined name 'logging' |
reviewbot | |
undefined name 'SCMError' |
reviewbot | |
local variable 'gitlab_msg' is assigned to but never used |
reviewbot | |
undefined name 'ref_name' |
reviewbot | |
'cache' imported but unused |
reviewbot | |
Col: 45 E128 continuation line under-indented for visual indent |
reviewbot | |
undefined name 'HTTPERRor' |
reviewbot | |
undefined name 'logging' |
reviewbot | |
undefined name 'SCMError' |
reviewbot | |
undefined name 'logging' |
reviewbot | |
undefined name 'logging' |
reviewbot | |
undefined name 'SCMError' |
reviewbot | |
undefined name 'logging' |
reviewbot | |
undefined name 'SCMError' |
reviewbot | |
local variable 'gitlab_msg' is assigned to but never used |
reviewbot | |
undefined name 'ref_name' |
reviewbot | |
'cache' imported but unused |
reviewbot | |
Col: 45 E128 continuation line under-indented for visual indent |
reviewbot | |
A lot of the functions you have in GitLabClient are unneccessary. I'm mostly referring to the http_get(), http_post() ones, but … |
JY jyuen | |
This is a GitHub private function, I don't believe we need to check rate limits for GitLab APIs, at least … |
JY jyuen | |
This is redundant with _api_get() that already exists. This will actually not work because GitLab APIs require authentication, see the … |
JY jyuen | |
Why not just if branch: since you do start = branch right before? |
JY jyuen | |
local variable 'response_info' is assigned to but never used |
reviewbot | |
Col: 45 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 9 E265 block comment should start with '# ' |
reviewbot | |
Col: 17 E113 unexpected indentation |
reviewbot | |
Col: 9 E265 block comment should start with '# ' |
reviewbot | |
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot | |
Col: 9 E265 block comment should start with '# ' |
reviewbot | |
'urllib2' imported but unused |
reviewbot | |
Please use %-formatting. |
brennie | |
Blank line between these. |
brennie | |
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()) |
brennie | |
Blank line between these. |
brennie | |
No comma on the last line -- it is a function call, not a list/dict |
brennie | |
I don't think these comments are needed since the code does a good job of explaining it already. |
JY jyuen | |
Blank line between these. |
brennie | |
This seems like good information for the function docstring. |
brennie | |
Blank line between these. |
brennie | |
Can we combine these? commit_api_url = '%s/repository/commits/%s?private_token=%s' % (repo_api_url, revision, private_token) |
JY jyuen | |
Have you tried this for the case where the current revision has no parents (i.e., it is the initial commit … |
brennie | |
local variable 'repo_id' is assigned to but never used |
reviewbot | |
Can we also combine these? |
JY jyuen | |
Col: 24 E127 continuation line over-indented for visual indent |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 61 W291 trailing whitespace |
reviewbot | |
Col: 30 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 30 E128 continuation line under-indented for visual indent |
reviewbot | |
Col: 76 W291 trailing whitespace |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 17 E126 continuation line over-indented for hanging indent |
reviewbot | |
Col: 17 E126 continuation line over-indented for hanging indent |
reviewbot | |
Col: 17 E126 continuation line over-indented for hanging indent |
reviewbot | |
Col: 34 E226 missing whitespace around arithmetic operator |
reviewbot | |
Alphabetize these. |
brennie | |
This sentence doesn't really add anything to the docstring. |
brennie | |
See my previous comment. Also instead of saying "The start" you should say "The start parameter". |
brennie | |
"The branch is optional. If it is not given, use the default branch." |
brennie | |
Just curious, but why are we only getting the first element of this response? This should be documented. |
brennie | |
Is item ever not a dict? If it is something else, what is it and why do we ignore it. … |
brennie | |
These comments don't really add anything. |
brennie | |
I mentioned this in a previous review, but there should be a blank line between these. It is a style … |
brennie | |
Reword this. You should mention that a long SHA (capitalized) is 40 characters. |
brennie | |
emove this line. |
brennie | |
Use all available line width (up to 79 characters). |
brennie | |
Can we use repo_api_url as an argument in the %-formatting expression? |
brennie | |
"a diff" |
brennie | |
Can you use < and > instead of " and " here? |
brennie | |
Can we use a %-formatting expression here? |
brennie | |
This seems like an issue prone way to strip the last two lines. Can we do a reverse search from … |
brennie | |
Blank line between these. |
brennie | |
This isn't a necessary comment. |
brennie | |
These comments don't add a lot to the code. |
brennie | |
See previous issues re: comments. |
brennie | |
See previous issues re: comments. |
brennie | |
This seems like a pretty critical problem. We need to start logging at 'start' if it's present, and 'branch' if … |
david | |
Col: 77 W291 trailing whitespace |
reviewbot | |
"commit ID". Remove "in here" |
brennie | |
No blank line here. |
brennie | |
This should be "Get a list" |
david | |
Should use single quotes. |
david | |
What else can ref be if it doesn't have a 'name' entry? |
brennie | |
Should be "Get a list" |
david | |
"commit ID" Remove "in here" |
brennie | |
Should use single quotes. |
david | |
Comments should start with a capital letter. We also don't "pretend it as a branch name", we use that SHA … |
david | |
Should use single quotes. |
david | |
Remove this blank line. |
david | |
Your first sentence here is run-on. Please change it to "The branch is optional. If it is not given..." |
david | |
Should use single quotes. |
david | |
The comma at the end of this line should be a period, and the next line should start a new … |
david | |
paginates -> paginate. |
david | |
"In prevent of removing the initial commit" is bad grammar. How about "To prevent removing the initial commit"? |
david | |
Can we define a constant for the page size? |
david | |
Can page_size be a constant on the class, like COMMITS_PER_PAGE ? |
brennie | |
Should be "Get the diff" |
david | |
Remove "In this function" Change "stands for the" to "is a" "commit ID" "(which is a long SHA) and consists … |
brennie | |
Your grammar here is a little off ("which is a long sha consists of 40 characters") -> should probably be … |
david | |
"SHA" |
david | |
Should use single quotes. |
david | |
"namespace" is one word. |
david | |
Should use single quotes. |
david | |
Should use single quotes. |
david | |
This comment doesn't really add anything, and it's the sort of thing which is likely to get out of date … |
david | |
This should define the string as bytes (prefix with a 'b' character) |
david | |
Comments should start with a capital letter. |
david | |
Comments should start with a capital letter. |
david | |
Comments should start with a capital letter. |
david | |
Col: 1 W191 indentation contains tabs |
reviewbot | |
Col: 1 E101 indentation contains mixed spaces and tabs |
reviewbot | |
Col: 1 E101 indentation contains mixed spaces and tabs |
reviewbot | |
Col: 1 W293 blank line contains whitespace |
reviewbot | |
Col: 1 E101 indentation contains mixed spaces and tabs |
reviewbot | |
Col: 1 W191 indentation contains tabs |
reviewbot | |
Col: 1 E101 indentation contains mixed spaces and tabs |
reviewbot | |
Col: 80 E501 line too long (81 > 79 characters) |
reviewbot | |
Col: 30 E251 unexpected spaces around keyword / parameter equals |
reviewbot | |
Col: 32 E251 unexpected spaces around keyword / parameter equals |
reviewbot | |
I don't think we need the note about "headers"--it's obvious from looking at the code, and this sort of comment … |
david | |
Add a period at the end of this comment. |
david | |
I don't think we need this comment. |
david | |
undefined name 'page_size' |
reviewbot | |
Can we change COMMITS_PER_PAGE to be 20, and then check if len(commits) == self.COMMITS_PER_PAGE? I think the "19" number can … |
david | |
In the case where there are only 20 commits in the repository, you WILL remove the initial commit. This needs … |
brennie | |
Christian suggested that this comment be: # The maximum number of commits returned from each call to get_commits() |
david | |
How about: # Ask GitLab for 21 commits per page. GitLab's API doesn't # include the parent IDs, so we … |
david | |
If you make the change I suggested to the comment text to page_size above, you can remove all but the … |
david |
-
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.
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 1) Make sure all trailing whitespace is cleaned up.
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 1) If functions aren't being used, we don't want to introduce them yet, even in commented-out form.
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 1) 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.
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+309 -2) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/hostingsvcs/github.py reviewboard/hostingsvcs/gitlab.py Tool: PEP8 Style Checker Processed Files: reviewboard/hostingsvcs/gitlab.py
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 2) Col: 47 E127 continuation line over-indented for visual indent
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 2) Col: 5 E265 block comment should start with '# '
-
-
-
-
-
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 2) Col: 31 E127 continuation line over-indented for visual indent
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 2) Col: 35 E128 continuation line under-indented for visual indent
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 2) Col: 35 E128 continuation line under-indented for visual indent
-
-
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 3) Col: 47 E127 continuation line over-indented for visual indent
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 3) Col: 5 E265 block comment should start with '# '
-
-
-
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 3) Col: 31 E127 continuation line over-indented for visual indent
-
-
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+266 -1) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/hostingsvcs/gitlab.py
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 4) Col: 45 E128 continuation line under-indented for visual indent
-
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+241 -1) |
-
Tool: Pyflakes Processed Files: reviewboard/hostingsvcs/gitlab.py Tool: PEP8 Style Checker Processed Files: reviewboard/hostingsvcs/gitlab.py
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 5) Col: 45 E128 continuation line under-indented for visual indent
-
-
-
-
-
-
-
-
-
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 5) local variable 'gitlab_msg' is assigned to but never used
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+240 -1) |
-
Tool: Pyflakes Processed Files: reviewboard/hostingsvcs/gitlab.py Tool: PEP8 Style Checker Processed Files: reviewboard/hostingsvcs/gitlab.py
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 6) Col: 45 E128 continuation line under-indented for visual indent
-
-
-
-
-
-
-
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 6) local variable 'gitlab_msg' is assigned to but never used
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+241 -2) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/hostingsvcs/gitlab.py Tool: Pyflakes Processed Files: reviewboard/hostingsvcs/gitlab.py
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 7) Col: 45 E128 continuation line under-indented for visual indent
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 7) local variable 'response_info' is assigned to but never used
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 7) 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.
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 7) 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.
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 7) 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.
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 7) Why not just
if branch:
since you dostart = branch
right before?
Description: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||
Diff: |
Revision 8 (+219 -2) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/hostingsvcs/gitlab.py
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 8) Col: 45 E128 continuation line under-indented for visual indent
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 8) Col: 9 E265 block comment should start with '# '
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 8) Col: 9 E265 block comment should start with '# '
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 8) Col: 9 E265 block comment should start with '# '
Testing Done: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||
Diff: |
Revision 9 (+100) |
-
Tool: Pyflakes Processed Files: reviewboard/hostingsvcs/gitlab.py Tool: PEP8 Style Checker Processed Files: reviewboard/hostingsvcs/gitlab.py
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 9) local variable 'repo_id' is assigned to but never used
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 9) Col: 24 E127 continuation line over-indented for visual indent
-
This is looking a lot cleaner! :)
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 9) I don't think these comments are needed since the code does a good job of explaining it already.
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 9) Can we combine these?
commit_api_url = '%s/repository/commits/%s?private_token=%s' % (repo_api_url, revision, private_token)
-
-
-
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 9) 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())
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 9) No comma on the last line -- it is a function call, not a list/dict
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 9) This seems like good information for the function docstring.
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 9) Have you tried this for the case where the current revision has no parents (i.e., it is the initial commit in the repository)?
Summary: |
|
|||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||
Diff: |
Revision 10 (+326) |
-
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
-
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 10) Col: 30 E128 continuation line under-indented for visual indent
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 10) Col: 30 E128 continuation line under-indented for visual indent
-
-
-
reviewboard/hostingsvcs/tests.py (Diff revision 10) Col: 17 E126 continuation line over-indented for hanging indent
-
reviewboard/hostingsvcs/tests.py (Diff revision 10) Col: 17 E126 continuation line over-indented for hanging indent
-
reviewboard/hostingsvcs/tests.py (Diff revision 10) Col: 17 E126 continuation line over-indented for hanging indent
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+326) |
-
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
Testing Done: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||
Diff: |
Revision 12 (+335) |
-
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
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 12) Col: 34 E226 missing whitespace around arithmetic operator
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+335) |
-
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> -
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 13) This sentence doesn't really add anything to the docstring.
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 13) See my previous comment. Also instead of saying "The start" you should say "The start parameter".
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 13) "The branch is optional. If it is not given, use the default branch."
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 13) Is
item
ever not adict
? 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. -
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 13) I mentioned this in a previous review, but there should be a blank line between these. It is a style consistency thing.
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 13) Reword this. You should mention that a long SHA (capitalized) is 40 characters.
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 13) Can we use
repo_api_url
as an argument in the%
-formatting expression? -
-
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 13) 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 anrsplit
withmaxsep=2
andsep='\n'
and only keep the first part? -
-
-
-
-
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 13) Just curious, but why are we only getting the first element of this response? This should be documented.
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 14 (+320) |
-
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
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revisions 13 - 14) Use all available line width (up to 79 characters).
-
Oops, forgot to edit my last review!
This is looking really good, but I'm going to wait on a second opinion before I sign off on it because I'm not intimately familiar with hosting services.
I do have one little nit, though. The summary will be the first line of the commit message once it lands so the first line of your description is redundant. No big deal.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 15 (+320) |
-
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
-
Description: |
|
||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||||||||||||||||||||
Diff: |
Revision 16 (+320) |
-
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
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 14) This seems like a pretty critical problem. We need to start logging at 'start' if it's present, and 'branch' if it's not, otherwise pagination will be completely wrong.
Please test with a repository that has > 30 commits and verify that pagination works right.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 17 (+334) |
-
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
-
-
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 17) What else can
ref
be if it doesn't have a'name'
entry? -
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 17) Remove "In this function"
Change "stands for the" to "is a"
"commit ID"
"(which is a long SHA) and consists of..."
-
-
-
-
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 17) Comments should start with a capital letter.
We also don't "pretend it as a branch name", we use that SHA as the latest commit to log from.
-
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 17) Your first sentence here is run-on. Please change it to "The branch is optional. If it is not given..."
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 17) The comma at the end of this line should be a period, and the next line should start a new sentence.
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 17) "In prevent of removing the initial commit" is bad grammar. How about "To prevent removing the initial commit"?
-
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 17) Your grammar here is a little off ("which is a long sha consists of 40 characters") -> should probably be "consisting". Also please capitalize "SHA"
-
-
-
-
-
-
reviewboard/hostingsvcs/tests.py (Diff revision 17) This comment doesn't really add anything, and it's the sort of thing which is likely to get out of date if we change the
get_commits
implementation. -
reviewboard/hostingsvcs/tests.py (Diff revision 17) This should define the string as bytes (prefix with a 'b' character)
-
-
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 18 (+331) |
-
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
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 18) Col: 1 E101 indentation contains mixed spaces and tabs
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 18) Col: 1 E101 indentation contains mixed spaces and tabs
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 18) Col: 1 E101 indentation contains mixed spaces and tabs
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 18) Col: 1 E101 indentation contains mixed spaces and tabs
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 18) Col: 80 E501 line too long (81 > 79 characters)
-
reviewboard/hostingsvcs/tests.py (Diff revision 18) Col: 30 E251 unexpected spaces around keyword / parameter equals
-
reviewboard/hostingsvcs/tests.py (Diff revision 18) Col: 32 E251 unexpected spaces around keyword / parameter equals
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 19 (+332) |
-
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
-
This is looking very complete! I have just a few comments on your comments.
Also, in your testing done, you say "Tested on local server, ran as expected." Please be specific about which manual test-cases you went through.
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 19) I don't think we need the note about "headers"--it's obvious from looking at the code, and this sort of comment can get out of date if we make a change to start using it.
-
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 20 (+329) |
-
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
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revisions 17 - 20) Can
page_size
be a constant on the class, likeCOMMITS_PER_PAGE
?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 21 (+331) |
-
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
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 22 (+331) |
-
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
-
Still waiting on this comment from a previous review:
In your testing done, you say "Tested on local server, ran as expected." Please be specific about which manual test-cases you went through.
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 22) Can we change COMMITS_PER_PAGE to be 20, and then check
if len(commits) == self.COMMITS_PER_PAGE
? I think the "19" number can be a bit confusing since GitLab is returning 20.
Testing Done: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
|||||||||
Diff: |
Revision 23 (+330) |
-
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
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 23) In the case where there are only 20 commits in the repository, you WILL remove the initial commit. This needs to be reworded to reflect that.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 24 (+337) |
-
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
-
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 24) Christian suggested that this comment be:
# The maximum number of commits returned from each call to get_commits()
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 24) How about:
# Ask GitLab for 21 commits per page. GitLab's API doesn't # include the parent IDs, so we use subsequent commits to fill # them in (allowing us to return 20 commits with parent IDs).
-
reviewboard/hostingsvcs/gitlab.py (Diff revision 24) If you make the change I suggested to the comment text to
page_size
above, you can remove all but the first line of this comment.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 25 (+337) |
-
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
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 26 (+334) |