Implement post-commit review request support for GitLab

Review Request #6872 — Created Feb. 1, 2015 and submitted

Information

Review Board
master
79f4156...

Reviewers

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

reviewbotreviewbot

That's a GitHub-specific thing.

chipx86chipx86

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

reviewbotreviewbot

Col: 6 W291 trailing whitespace

reviewbotreviewbot

Make sure all trailing whitespace is cleaned up.

chipx86chipx86

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

chipx86chipx86

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

None of these functions are doing anything. Having them here is just going to add confusion. Make sure we're only …

chipx86chipx86

Col: 28 E241 multiple spaces after ','

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

These are all missing docstrings.

chipx86chipx86

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

reviewbotreviewbot

Col: 35 E703 statement ends with a semicolon

reviewbotreviewbot

Col: 13 E113 unexpected indentation

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 6 W291 trailing whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 72 W291 trailing whitespace

reviewbotreviewbot

Col: 28 E241 multiple spaces after ','

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 73 W291 trailing whitespace

reviewbotreviewbot

Col: 35 E703 statement ends with a semicolon

reviewbotreviewbot

Col: 13 E113 unexpected indentation

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 6 W291 trailing whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 72 W291 trailing whitespace

reviewbotreviewbot

Col: 28 E241 multiple spaces after ','

reviewbotreviewbot

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

reviewbotreviewbot

Col: 73 W291 trailing whitespace

reviewbotreviewbot

Col: 35 E703 statement ends with a semicolon

reviewbotreviewbot

Col: 13 E113 unexpected indentation

reviewbotreviewbot

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

reviewbotreviewbot

Col: 6 W291 trailing whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

'cache' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

Col: 6 W291 trailing whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

undefined name 'HTTPERRor'

reviewbotreviewbot

undefined name 'logging'

reviewbotreviewbot

undefined name 'SCMError'

reviewbotreviewbot

undefined name 'logging'

reviewbotreviewbot

undefined name 'logging'

reviewbotreviewbot

undefined name 'SCMError'

reviewbotreviewbot

undefined name 'logging'

reviewbotreviewbot

undefined name 'SCMError'

reviewbotreviewbot

local variable 'gitlab_msg' is assigned to but never used

reviewbotreviewbot

undefined name 'ref_name'

reviewbotreviewbot

'cache' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

undefined name 'HTTPERRor'

reviewbotreviewbot

undefined name 'logging'

reviewbotreviewbot

undefined name 'SCMError'

reviewbotreviewbot

undefined name 'logging'

reviewbotreviewbot

undefined name 'logging'

reviewbotreviewbot

undefined name 'SCMError'

reviewbotreviewbot

undefined name 'logging'

reviewbotreviewbot

undefined name 'SCMError'

reviewbotreviewbot

local variable 'gitlab_msg' is assigned to but never used

reviewbotreviewbot

undefined name 'ref_name'

reviewbotreviewbot

'cache' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 17 E113 unexpected indentation

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

'urllib2' imported but unused

reviewbotreviewbot

Please use %-formatting.

brenniebrennie

Blank line between these.

brenniebrennie

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())

brenniebrennie

Blank line between these.

brenniebrennie

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

brenniebrennie

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.

brenniebrennie

This seems like good information for the function docstring.

brenniebrennie

Blank line between these.

brenniebrennie

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 …

brenniebrennie

local variable 'repo_id' is assigned to but never used

reviewbotreviewbot

Can we also combine these?

JY jyuen

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

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 61 W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 76 W291 trailing whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 34 E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Alphabetize these.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

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

brenniebrennie

Just curious, but why are we only getting the first element of this response? This should be documented.

brenniebrennie

Is item ever not a dict? If it is something else, what is it and why do we ignore it. …

brenniebrennie

These comments don't really add anything.

brenniebrennie

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

brenniebrennie

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

brenniebrennie

emove this line.

brenniebrennie

Use all available line width (up to 79 characters).

brenniebrennie

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

brenniebrennie

"a diff"

brenniebrennie

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

brenniebrennie

Can we use a %-formatting expression here?

brenniebrennie

This seems like an issue prone way to strip the last two lines. Can we do a reverse search from …

brenniebrennie

Blank line between these.

brenniebrennie

This isn't a necessary comment.

brenniebrennie

These comments don't add a lot to the code.

brenniebrennie

See previous issues re: comments.

brenniebrennie

See previous issues re: comments.

brenniebrennie

This seems like a pretty critical problem. We need to start logging at 'start' if it's present, and 'branch' if …

daviddavid

Col: 77 W291 trailing whitespace

reviewbotreviewbot

"commit ID". Remove "in here"

brenniebrennie

No blank line here.

brenniebrennie

This should be "Get a list"

daviddavid

Should use single quotes.

daviddavid

What else can ref be if it doesn't have a 'name' entry?

brenniebrennie

Should be "Get a list"

daviddavid

"commit ID" Remove "in here"

brenniebrennie

Should use single quotes.

daviddavid

Comments should start with a capital letter. We also don't "pretend it as a branch name", we use that SHA …

daviddavid

Should use single quotes.

daviddavid

Remove this blank line.

daviddavid

Your first sentence here is run-on. Please change it to "The branch is optional. If it is not given..."

daviddavid

Should use single quotes.

daviddavid

The comma at the end of this line should be a period, and the next line should start a new …

daviddavid

paginates -> paginate.

daviddavid

"In prevent of removing the initial commit" is bad grammar. How about "To prevent removing the initial commit"?

daviddavid

Can we define a constant for the page size?

daviddavid

Can page_size be a constant on the class, like COMMITS_PER_PAGE ?

brenniebrennie

Should be "Get the diff"

daviddavid

Remove "In this function" Change "stands for the" to "is a" "commit ID" "(which is a long SHA) and consists …

brenniebrennie

Your grammar here is a little off ("which is a long sha consists of 40 characters") -> should probably be …

daviddavid

"SHA"

daviddavid

Should use single quotes.

daviddavid

"namespace" is one word.

daviddavid

Should use single quotes.

daviddavid

Should use single quotes.

daviddavid

This comment doesn't really add anything, and it's the sort of thing which is likely to get out of date …

daviddavid

This should define the string as bytes (prefix with a 'b' character)

daviddavid

Comments should start with a capital letter.

daviddavid

Comments should start with a capital letter.

daviddavid

Comments should start with a capital letter.

daviddavid

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 1 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

Col: 1 W191 indentation contains tabs

reviewbotreviewbot

Col: 1 E101 indentation contains mixed spaces and tabs

reviewbotreviewbot

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

reviewbotreviewbot

Col: 30 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 32 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

I don't think we need the note about "headers"--it's obvious from looking at the code, and this sort of comment …

daviddavid

Add a period at the end of this comment.

daviddavid

I don't think we need this comment.

daviddavid

undefined name 'page_size'

reviewbotreviewbot

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 …

daviddavid

In the case where there are only 20 commits in the repository, you WILL remove the initial commit. This needs …

brenniebrennie

Christian suggested that this comment be: # The maximum number of commits returned from each call to get_commits()

daviddavid

How about: # Ask GitLab for 21 commits per page. GitLab's API doesn't # include the parent IDs, so we …

daviddavid

If you make the change I suggested to the comment text to page_size above, you can remove all but the …

daviddavid
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/hostingsvcs/gitlab.py
    
    
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     
    Col: 47
     E127 continuation line over-indented for visual indent
    
  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     
    Col: 5
     E265 block comment should start with '# '
    
  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     
    Col: 6
     W291 trailing whitespace
    
  5. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  6. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  7. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  8. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     
    Col: 28
     E241 multiple spaces after ','
    
  9. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     
    Col: 35
     E128 continuation line under-indented for visual indent
    
  10. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     
    Col: 35
     E128 continuation line under-indented for visual indent
    
  11. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     
    Col: 49
     E127 continuation line over-indented for visual indent
    
  12. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     
    Col: 35
     E703 statement ends with a semicolon
    
  13. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     
    Col: 13
     E113 unexpected indentation
    
  14. 
      
Chester
chipx86
  1. 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.

  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     
     

    That's a GitHub-specific thing.

  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    Make sure all trailing whitespace is cleaned up.

  4. 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.

  5. 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.

  6. reviewboard/hostingsvcs/gitlab.py (Diff revision 1)
     
     

    These are all missing docstrings.

  7. 
      
Chester
Chester
Chester
Chester
Chester
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/hostingsvcs/github.py
        reviewboard/hostingsvcs/gitlab.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/hostingsvcs/gitlab.py
    
    
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
    Col: 47
     E127 continuation line over-indented for visual indent
    
  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
    Col: 5
     E265 block comment should start with '# '
    
  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
    Col: 6
     W291 trailing whitespace
    
  5. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  6. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
    Col: 5
     E303 too many blank lines (2)
    
  7. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  8. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
    Col: 72
     W291 trailing whitespace
    
  9. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
    Col: 28
     E241 multiple spaces after ','
    
  10. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
    Col: 31
     E127 continuation line over-indented for visual indent
    
  11. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
    Col: 35
     E128 continuation line under-indented for visual indent
    
  12. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
    Col: 35
     E128 continuation line under-indented for visual indent
    
  13. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
    Col: 73
     W291 trailing whitespace
    
  14. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
    Col: 35
     E703 statement ends with a semicolon
    
  15. reviewboard/hostingsvcs/gitlab.py (Diff revision 2)
     
     
    Col: 13
     E113 unexpected indentation
    
  16. reviewboard/hostingsvcs/gitlab.py (Diff revision 3)
     
     
    Col: 47
     E127 continuation line over-indented for visual indent
    
  17. reviewboard/hostingsvcs/gitlab.py (Diff revision 3)
     
     
    Col: 5
     E265 block comment should start with '# '
    
  18. reviewboard/hostingsvcs/gitlab.py (Diff revision 3)
     
     
    Col: 6
     W291 trailing whitespace
    
  19. reviewboard/hostingsvcs/gitlab.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  20. reviewboard/hostingsvcs/gitlab.py (Diff revision 3)
     
     
    Col: 72
     W291 trailing whitespace
    
  21. reviewboard/hostingsvcs/gitlab.py (Diff revision 3)
     
     
    Col: 28
     E241 multiple spaces after ','
    
  22. reviewboard/hostingsvcs/gitlab.py (Diff revision 3)
     
     
    Col: 31
     E127 continuation line over-indented for visual indent
    
  23. reviewboard/hostingsvcs/gitlab.py (Diff revision 3)
     
     
    Col: 73
     W291 trailing whitespace
    
  24. reviewboard/hostingsvcs/gitlab.py (Diff revision 3)
     
     
    Col: 35
     E703 statement ends with a semicolon
    
  25. reviewboard/hostingsvcs/gitlab.py (Diff revision 3)
     
     
    Col: 13
     E113 unexpected indentation
    
  26. 
      
Chester
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/hostingsvcs/gitlab.py
    
    
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 4)
     
     
    Col: 45
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 4)
     
     
    Col: 6
     W291 trailing whitespace
    
  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 4)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  5. 
      
Chester
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/hostingsvcs/gitlab.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/hostingsvcs/gitlab.py
    
    
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     'cache' imported but unused
    
  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
    Col: 45
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
    Col: 6
     W291 trailing whitespace
    
  5. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  6. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     undefined name 'HTTPERRor'
    
  7. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     undefined name 'logging'
    
  8. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     undefined name 'SCMError'
    
  9. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     undefined name 'logging'
    
  10. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     undefined name 'logging'
    
  11. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     undefined name 'SCMError'
    
  12. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     undefined name 'logging'
    
  13. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     undefined name 'SCMError'
    
  14. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     local variable 'gitlab_msg' is assigned to but never used
    
  15. reviewboard/hostingsvcs/gitlab.py (Diff revision 5)
     
     
     undefined name 'ref_name'
    
  16. 
      
Chester
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/hostingsvcs/gitlab.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/hostingsvcs/gitlab.py
    
    
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 6)
     
     
     'cache' imported but unused
    
  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 6)
     
     
    Col: 45
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 6)
     
     
     undefined name 'HTTPERRor'
    
  5. reviewboard/hostingsvcs/gitlab.py (Diff revision 6)
     
     
     undefined name 'logging'
    
  6. reviewboard/hostingsvcs/gitlab.py (Diff revision 6)
     
     
     undefined name 'SCMError'
    
  7. reviewboard/hostingsvcs/gitlab.py (Diff revision 6)
     
     
     undefined name 'logging'
    
  8. reviewboard/hostingsvcs/gitlab.py (Diff revision 6)
     
     
     undefined name 'logging'
    
  9. reviewboard/hostingsvcs/gitlab.py (Diff revision 6)
     
     
     undefined name 'SCMError'
    
  10. reviewboard/hostingsvcs/gitlab.py (Diff revision 6)
     
     
     undefined name 'logging'
    
  11. reviewboard/hostingsvcs/gitlab.py (Diff revision 6)
     
     
     undefined name 'SCMError'
    
  12. reviewboard/hostingsvcs/gitlab.py (Diff revision 6)
     
     
     local variable 'gitlab_msg' is assigned to but never used
    
  13. reviewboard/hostingsvcs/gitlab.py (Diff revision 6)
     
     
     undefined name 'ref_name'
    
  14. 
      
Chester
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/hostingsvcs/gitlab.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/hostingsvcs/gitlab.py
    
    
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 7)
     
     
     'cache' imported but unused
    
  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 7)
     
     
    Col: 45
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 7)
     
     
     local variable 'response_info' is assigned to but never used
    
  5. 
      
JY
  1. 
      
  2. 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.

    1. I agree, just because I wasn't sure if I need to override these function in GitLabClient class. I will work on this. Thank you!

  3. 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.

    1. I haven't seen this function from GitLab API document either! But I don't know if we still need to check the rate limits in some way... Maybe just remove this function. Thank you!

  4. 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.

  5. reviewboard/hostingsvcs/gitlab.py (Diff revision 7)
     
     

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

  6. 
      
Chester
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/hostingsvcs/gitlab.py
    
    
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 8)
     
     
    Col: 45
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 8)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 8)
     
     
    Col: 17
     E113 unexpected indentation
    
  5. reviewboard/hostingsvcs/gitlab.py (Diff revision 8)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  6. reviewboard/hostingsvcs/gitlab.py (Diff revision 8)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  7. reviewboard/hostingsvcs/gitlab.py (Diff revision 8)
     
     
    Col: 9
     E265 block comment should start with '# '
    
  8. 
      
Chester
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/hostingsvcs/gitlab.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/hostingsvcs/gitlab.py
    
    
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 9)
     
     
     'urllib2' imported but unused
    
    1. will be removed in next request

  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 9)
     
     
     local variable 'repo_id' is assigned to but never used
    
  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 9)
     
     
    Col: 24
     E127 continuation line over-indented for visual indent
    
  5. 
      
JY
  1. This is looking a lot cleaner! :)

  2. 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.

    1. Probably, but I think I will leave these comments over there at this moment.

  3. 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)

    1. I tried but it turned out I would still need the same number of lines but more difficult to read after combination. So separation is better in here

    2. It isn't about number of lines. It takes more time to do string appends than doing %-formatting. Since you're doing some formatting anyway, you should just use that instead.

    3. Good to know! I will fix it.

  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 9)
     
     
     
     

    Can we also combine these?

    1. same as previous one, it would be nicer to separate them.

    2. See my comment on the previous issue.

  5. 
      
brennie
  1. 
      
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 9)
     
     
     
     

    Please use %-formatting.

    1. so like this repo_api_url += "/repository/branches?private_token=%s" % (private_token,)?

    2. No, I mean don't append strings, just use a single %-formatting expression.

  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 9)
     
     
     

    Blank line between these.

  4. 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())
    
  5. reviewboard/hostingsvcs/gitlab.py (Diff revision 9)
     
     
     

    Blank line between these.

  6. reviewboard/hostingsvcs/gitlab.py (Diff revision 9)
     
     
     
     
     
     

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

    1. It will have been fixed in next review request.

  7. reviewboard/hostingsvcs/gitlab.py (Diff revision 9)
     
     
     

    Blank line between these.

    1. I tlhink it might be better to keep them like this, I posted the reason in another comment.

  8. reviewboard/hostingsvcs/gitlab.py (Diff revision 9)
     
     
     

    This seems like good information for the function docstring.

  9. reviewboard/hostingsvcs/gitlab.py (Diff revision 9)
     
     
     

    Blank line between these.

    1. I think it is better to make variable assignment and where to use the variable would be better sytle. If they are irrelevant parts, I will definitely separate them with one blank space.

    2. This is our style guide.

  10. 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)?

    1. the first commit (i.e. no parent) can not be fetch by diff request, I don't know why, it looks like kind of git thing.

  11. 
      
Chester
reviewbot
  1. 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
    
    
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 10)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 10)
     
     
    Col: 61
     W291 trailing whitespace
    
  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 10)
     
     
    Col: 30
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/hostingsvcs/gitlab.py (Diff revision 10)
     
     
    Col: 30
     E128 continuation line under-indented for visual indent
    
  6. reviewboard/hostingsvcs/gitlab.py (Diff revision 10)
     
     
    Col: 76
     W291 trailing whitespace
    
  7. reviewboard/hostingsvcs/tests.py (Diff revision 10)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  8. reviewboard/hostingsvcs/tests.py (Diff revision 10)
     
     
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  9. reviewboard/hostingsvcs/tests.py (Diff revision 10)
     
     
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  10. reviewboard/hostingsvcs/tests.py (Diff revision 10)
     
     
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  11. 
      
Chester
reviewbot
  1. 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
    
    
  2. 
      
Chester
reviewbot
  1. 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
    
    
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 12)
     
     
    Col: 34
     E226 missing whitespace around arithmetic operator
    
  3. 
      
Chester
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. <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>

  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 13)
     
     
     
     

    Alphabetize these.

  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 13)
     
     

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

    1. How about "This will perform an API request to fetch a list branches"? or just delete it?

    2. "a list of branches." But yes, that would be a good addition.

  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 13)
     
     

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

    1. yes, 'start' is the parameter. Do I use <> or '' to clarify it is a parameter?

    2. You don't need either. Just mention "The start parameter"

  5. reviewboard/hostingsvcs/gitlab.py (Diff revision 13)
     
     

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

  6. reviewboard/hostingsvcs/gitlab.py (Diff revision 13)
     
     

    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.

    1. In the GitLab API response, the last argument's type is 'instance', anyway, since I have commits = self._api_get(repo_api_url)[0], commits won't have it. I will remove this conditional statement.

  7. reviewboard/hostingsvcs/gitlab.py (Diff revision 13)
     
     
     
     
     

    These comments don't really add anything.

  8. 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.

  9. reviewboard/hostingsvcs/gitlab.py (Diff revision 13)
     
     
     

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

  10. reviewboard/hostingsvcs/gitlab.py (Diff revision 13)
     
     

    emove this line.

  11. reviewboard/hostingsvcs/gitlab.py (Diff revision 13)
     
     
     
     

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

  12. reviewboard/hostingsvcs/gitlab.py (Diff revision 13)
     
     

    "a diff"

  13. reviewboard/hostingsvcs/gitlab.py (Diff revision 13)
     
     

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

  14. reviewboard/hostingsvcs/gitlab.py (Diff revision 13)
     
     

    Can we use a %-formatting expression here?

  15. 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 an rsplit with maxsep=2 and sep='\n' and only keep the first part?

    1. your strategy is way nicer!

  16. reviewboard/hostingsvcs/tests.py (Diff revision 13)
     
     
     

    Blank line between these.

  17. reviewboard/hostingsvcs/tests.py (Diff revision 13)
     
     

    This isn't a necessary comment.

  18. reviewboard/hostingsvcs/tests.py (Diff revision 13)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    These comments don't add a lot to the code.

  19. reviewboard/hostingsvcs/tests.py (Diff revision 13)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    See previous issues re: comments.

  20. reviewboard/hostingsvcs/tests.py (Diff revision 13)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    See previous issues re: comments.

  21. 
      
brennie
  1. 
      
  2. 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.

    1. The GitLab API will return a tuple consists of two elements, the first one is a list of commits, and the other one is instance type object containing all kinds of headers. I will document this information

  3. 
      
Chester
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/hostingsvcs/gitlab.py (Diff revisions 13 - 14)
     
     
     

    Use all available line width (up to 79 characters).

  3. 
      
brennie
  1. 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.

  2. 
      
Chester
reviewbot
  1. 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
    
    
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 15)
     
     
    Col: 77
     W291 trailing whitespace
    
  3. 
      
Chester
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. 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.

  3. 
      
Chester
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/hostingsvcs/gitlab.py (Diff revisions 16 - 17)
     
     

    "commit ID".

    Remove "in here"

  3. reviewboard/hostingsvcs/gitlab.py (Diff revisions 16 - 17)
     
     

    No blank line here.

  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    What else can ref be if it doesn't have a 'name' entry?

    1. According to GitLab API, responses of fetching branches will have 'name ' in it. However, becasue I am not sure if GitLab might have otther API operations with respect to branches, I put the conditional statement in here.

  5. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    "commit ID"

    Remove "in here"

  6. 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..."

    1. I will submit a final version with these fiexed after being reviewed by one more person.

  7. 
      
david
  1. 
      
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    This should be "Get a list"

  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    Should use single quotes.

  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    Should be "Get a list"

  5. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    Should use single quotes.

  6. 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.

  7. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    Should use single quotes.

  8. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    Remove this blank line.

  9. 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..."

  10. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    Should use single quotes.

  11. 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.

  12. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    paginates -> paginate.

  13. 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"?

  14. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    Can we define a constant for the page size?

  15. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    Should be "Get the diff"

  16. 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"

  17. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     
  18. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    Should use single quotes.

  19. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    "namespace" is one word.

  20. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    Should use single quotes.

  21. reviewboard/hostingsvcs/gitlab.py (Diff revision 17)
     
     

    Should use single quotes.

  22. 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.

  23. reviewboard/hostingsvcs/tests.py (Diff revision 17)
     
     

    This should define the string as bytes (prefix with a 'b' character)

  24. reviewboard/hostingsvcs/tests.py (Diff revision 17)
     
     

    Comments should start with a capital letter.

  25. reviewboard/hostingsvcs/tests.py (Diff revision 17)
     
     

    Comments should start with a capital letter.

  26. reviewboard/hostingsvcs/tests.py (Diff revision 17)
     
     

    Comments should start with a capital letter.

  27. 
      
Chester
reviewbot
  1. 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
    
    
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 18)
     
     
    Col: 1
     W191 indentation contains tabs
    
  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 18)
     
     
    Col: 1
     E101 indentation contains mixed spaces and tabs
    
    1. I don't know why it looks fine in vim...

  4. reviewboard/hostingsvcs/gitlab.py (Diff revision 18)
     
     
    Col: 1
     E101 indentation contains mixed spaces and tabs
    
  5. reviewboard/hostingsvcs/gitlab.py (Diff revision 18)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  6. reviewboard/hostingsvcs/gitlab.py (Diff revision 18)
     
     
    Col: 1
     E101 indentation contains mixed spaces and tabs
    
  7. reviewboard/hostingsvcs/gitlab.py (Diff revision 18)
     
     
    Col: 1
     W191 indentation contains tabs
    
  8. reviewboard/hostingsvcs/gitlab.py (Diff revision 18)
     
     
    Col: 1
     E101 indentation contains mixed spaces and tabs
    
  9. reviewboard/hostingsvcs/gitlab.py (Diff revision 18)
     
     
    Col: 80
     E501 line too long (81 > 79 characters)
    
  10. reviewboard/hostingsvcs/tests.py (Diff revision 18)
     
     
    Col: 30
     E251 unexpected spaces around keyword / parameter equals
    
  11. reviewboard/hostingsvcs/tests.py (Diff revision 18)
     
     
    Col: 32
     E251 unexpected spaces around keyword / parameter equals
    
  12. 
      
Chester
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 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.

  2. 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.

  3. reviewboard/hostingsvcs/gitlab.py (Diff revision 19)
     
     

    Add a period at the end of this comment.

  4. reviewboard/hostingsvcs/tests.py (Diff revision 19)
     
     

    I don't think we need this comment.

    1. I left some comments to help me understand the code, I will remove them.

  5. 
      
Chester
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/hostingsvcs/gitlab.py (Diff revisions 17 - 20)
     
     

    Can page_size be a constant on the class, like COMMITS_PER_PAGE ?

    1. Because the variable is used only in this function, we should probably keep its scope as small as possible. And other developers will have to scroll up to see the definition if we define it in class scope.

    2. Except that we're not using it as a variable, we are using it as a constant. This is how we use constants throughout the rest of the code base.

    3. If so, I will change put the definition afater name.

    4. Numbers with meaning hard-coded into the body of a function are called "magic numbers." These are usually not a good thing to have, because it becomes less clear over time what the number is for. As code grows, it also tends to lead to duplication of logic (for instance, if more than one function needs to use the same number, these values can get out of sync). In particular, when it's not a round number like 10 or 20, but something like 19 it's less clear.

      This should definitely be a constant on the class. The value should also be documented, since again, 19 is a very specific value that clearly carries some meaning.

  3. 
      
Chester
reviewbot
  1. 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
    
    
  2. reviewboard/hostingsvcs/gitlab.py (Diff revision 21)
     
     
     undefined name 'page_size'
    
  3. 
      
Chester
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 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.

    1. sorry, I forgot the very first two lines. I am updating it

  2. 
      
david
  1. 
      
  2. 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.

  3. 
      
Chester
reviewbot
  1. 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
    
    
  2. 
      
brennie
  1. 
      
  2. 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.

  3. 
      
Chester
reviewbot
  1. 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
    
    
  2. 
      
david
  1. 
      
  2. 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()
    
  3. 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).
    
  4. 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.

    1. It doesn't look like you made this change in your latest revision. I'm going to reopen the issue.

  5. 
      
Chester
reviewbot
  1. 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
    
    
  2. 
      
Chester
reviewbot
  1. 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
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
Chester
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (c6f90cf)
Loading...