• 
      

    Add post-commit support for BitBucket

    Review Request #7513 — Created July 10, 2015 and submitted

    Information

    Review Board
    release-2.5.x

    Reviewers

    Post-commit review request creation is now enabled for the BitBucket
    hosting service. The BitBucket hosting service class is now able to
    fetch lists of branches, lists of change metadata, and individual diffs
    from the BitBucket API.

    BitBucket's API does not support fetching files by their blob IDs and
    the base_commit_id parameter (which indicates the commit ID of the
    revision we want the file at) must be used. This field was added to the
    reviewboard.scmtools.core.Commit class so that it could be used when
    fetching commits from the API and to create review requests. Without
    this parameter, review request creation will fail (as all file
    existence checks will fail).

    Added a BitBucket hosted repository on my development server. I was
    able to create post-commit review requests.

    Description From Last Updated

    'markdown_escape' imported but unused

    reviewbot reviewbot

    We repeat the path building for repositories/%s/%s/ multiple times. Let's pull that part out into a utility function. Maybe build_repository_api_path, …

    chipx86 chipx86

    Rather than making these calls like this, can you separate them out into: rsp = self._api_get(url) return rsp['name'] While more …

    chipx86 chipx86

    This is an even better example of where separating is going to be easier to debug if things ever go …

    chipx86 chipx86

    We're doing this or twice. How about: start = start or branch if start: ...

    chipx86 chipx86

    Can you use keyword arguments for each parameter to Commit?

    chipx86 chipx86

    This is worth having a documented utility function for, as it's non-obvious what's going on.

    chipx86 chipx86

    Can you add a comment saying that the commit will be added to the change in the call to get_commits? …

    chipx86 chipx86

    Same here about keyword arguments.

    chipx86 chipx86

    Col: 5 E303 too many blank lines (2)

    reviewbot reviewbot
    reviewbot
    1. Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/models/base_review_request_details.py
          reviewboard/scmtools/core.py
          reviewboard/hostingsvcs/bitbucket.py
      
      
      
      Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/models/base_review_request_details.py
          reviewboard/scmtools/core.py
          reviewboard/hostingsvcs/bitbucket.py
      
      
    2. Show all issues
       'markdown_escape' imported but unused
      
    3. 
        
    brennie
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/models/base_review_request_details.py
          reviewboard/scmtools/core.py
          reviewboard/hostingsvcs/bitbucket.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/models/base_review_request_details.py
          reviewboard/scmtools/core.py
          reviewboard/hostingsvcs/bitbucket.py
      
      
    2. 
        
    brennie
    chipx86
    1. 
        
    2. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
       
       
       
       
      Show all issues

      We repeat the path building for repositories/%s/%s/ multiple times. Let's pull that part out into a utility function. Maybe build_repository_api_path, which could take the repository and anything that would appear after that base path.

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

      Rather than making these calls like this, can you separate them out into:

      rsp = self._api_get(url)
      
      return rsp['name']
      

      While more lengthy, it's also more readable and easier to debug this way. There's two distinct operations:

      1. We're fetching API data.
      2. We're extracting a value and returning it.

      In the event that there's an exception (let's say a KeyError) pointing to one of these lines, we'll know that it's due to either the API fetching, or the key extraction, at a glance.

      Same below.

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

      This is an even better example of where separating is going to be easier to debug if things ever go wrong, since Python considers this entire expression to be one statement and will place the exception's line at the last line of this.

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

      We're doing this or twice. How about:

      start = start or branch
      
      if start:
          ...
      
    6. reviewboard/hostingsvcs/bitbucket.py (Diff revision 2)
       
       
      Show all issues

      Can you use keyword arguments for each parameter to Commit?

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

      This is worth having a documented utility function for, as it's non-obvious what's going on.

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

      Can you add a comment saying that the commit will be added to the change in the call to get_commits? I had to dig through the code to figure that out (originally I thought we were missing a call).

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

      Same here about keyword arguments.

    10. 
        
    chipx86
    1. Oh, can you also flesh out the description a bit? This should go into more detail on what this allows, briefly what's involved in getting it to work, and the work needed to plumb the base_commit_id stuff through.

    2. 
        
    brennie
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/models/base_review_request_details.py
          reviewboard/scmtools/core.py
          reviewboard/hostingsvcs/bitbucket.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/models/base_review_request_details.py
          reviewboard/scmtools/core.py
          reviewboard/hostingsvcs/bitbucket.py
      
      
    2. reviewboard/hostingsvcs/bitbucket.py (Diff revision 3)
       
       
      Show all issues
      Col: 5
       E303 too many blank lines (2)
      
    3. 
        
    brennie
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          reviewboard/reviews/models/base_review_request_details.py
          reviewboard/scmtools/core.py
          reviewboard/hostingsvcs/bitbucket.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          reviewboard/reviews/models/base_review_request_details.py
          reviewboard/scmtools/core.py
          reviewboard/hostingsvcs/bitbucket.py
      
      
    2. 
        
    chipx86
    1. Ship It!
    2. 
        
    brennie
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-2.5.x (478c095)