[WIP] HostingServices: Bitbucket get_remote_repositories, get_commits, get_branches

Review Request #5450 — Created Feb. 10, 2014 and discarded

Information

Review Board
master

Reviewers

Added get_remote_repositories, get_commits, get_branches for Bitbucket

Command line testing done, unit tests will be added after tests in request #5432 are approved.

Description From Last Updated

Same as my other review concern - can we guarantee that this will exit without putting our full trust in …

mike_conleymike_conley

No space before the """.

chipx86chipx86

Can be one statement.

chipx86chipx86

Blank line before this.

chipx86chipx86

Blank line before this.

chipx86chipx86

BLANK LINE BEFORE THIS.

chipx86chipx86

One variable setting per line. Same below.

chipx86chipx86

Blank line between these.

chipx86chipx86

next is a reserved word.

chipx86chipx86

We don't use the if .. else .. form in our code. Should just be a standard if statement.

chipx86chipx86

The """ should go on the previous line.

chipx86chipx86

Blank line before this.

chipx86chipx86

Blank line before this.

chipx86chipx86

No u prefixes.

chipx86chipx86

And this.

chipx86chipx86

A quicker and nicer way of doing this is: results = [ Branch(...) for name, info in six.iteritems(branches) ] You …

chipx86chipx86

You can just do: if not results:

chipx86chipx86

And this.

chipx86chipx86

This line is too long. Maybe shorten the summary and go into more detail in the description.

chipx86chipx86

Blank line above.

chipx86chipx86

Blank line before this.

chipx86chipx86

Did you mean to capitalize all of "THE" ?

chipx86chipx86

And this.

chipx86chipx86

Blank line before this.

chipx86chipx86
mike_conley
  1. 
      
  2. reviewboard/hostingsvcs/bitbucket.py (Diff revision 1)
     
     
    Show all issues

    Same as my other review concern - can we guarantee that this will exit without putting our full trust in Bitbucket to do the right thing?

  3. 
      
OL
OL
OL
OL
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    No space before the """.

  3. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
     
    Show all issues

    Can be one statement.

  4. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    Blank line before this.

  5. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    Blank line before this.

  6. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    BLANK LINE BEFORE THIS.

  7. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    One variable setting per line.

    Same below.

  8. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
     
    Show all issues

    Blank line between these.

  9. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    next is a reserved word.

  10. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    We don't use the if .. else .. form in our code. Should just be a standard if statement.

  11. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
     
    Show all issues

    The """ should go on the previous line.

  12. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    Blank line before this.

  13. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    Blank line before this.

  14. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    No u prefixes.

  15. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    And this.

  16. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
     
     
     
    Show all issues

    A quicker and nicer way of doing this is:

    results = [
        Branch(...)
        for name, info in six.iteritems(branches)
    ]
    

    You then won't need to set results = [] above.

    Also note the six.iteritems. We use a module called six to provide compatibility with Python 3. One of the things we must use it for is any iter* function.

    If you do any iteration anywhere else in your code (I may have missed these), please update them to use six.

  17. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    You can just do:

    if not results:
    
  18. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    And this.

  19. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    This line is too long. Maybe shorten the summary and go into more detail in the description.

  20. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    Blank line above.

  21. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    Blank line before this.

  22. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    Did you mean to capitalize all of "THE" ?

  23. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    And this.

  24. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
     
     
    Show all issues

    Blank line before this.

  25. 
      
OL
OL
Review request changed

Status: Discarded

Loading...