-
-
reviewboard/reviews/models/base_review_request_details.py (Diff revision 1) 'markdown_escape' imported but unused
Add post-commit support for BitBucket
Review Request #7513 — Created July 10, 2015 and submitted
Post-commit review request creation is now enabled for the BitBucket
hosting service. TheBitBuckethosting 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
thebase_commit_idparameter (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.Commitclass 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
|
|
|
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
|
|
|
Rather than making these calls like this, can you separate them out into: rsp = self._api_get(url) return rsp['name'] While more … |
chipx86
|
|
|
This is an even better example of where separating is going to be easier to debug if things ever go … |
chipx86
|
|
|
We're doing this or twice. How about: start = start or branch if start: ... |
chipx86
|
|
|
Can you use keyword arguments for each parameter to Commit? |
chipx86
|
|
|
This is worth having a documented utility function for, as it's non-obvious what's going on. |
chipx86
|
|
|
Can you add a comment saying that the commit will be added to the change in the call to get_commits? … |
chipx86
|
|
|
Same here about keyword arguments. |
chipx86
|
|
|
Col: 5 E303 too many blank lines (2) |
reviewbot
|
-
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
-
-
reviewboard/hostingsvcs/bitbucket.py (Diff revision 2) We repeat the path building for
repositories/%s/%s/multiple times. Let's pull that part out into a utility function. Maybebuild_repository_api_path, which could take therepositoryand anything that would appear after that base path. -
reviewboard/hostingsvcs/bitbucket.py (Diff revision 2) 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:
- We're fetching API data.
- 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.
-
reviewboard/hostingsvcs/bitbucket.py (Diff revision 2) 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.
-
reviewboard/hostingsvcs/bitbucket.py (Diff revision 2) We're doing this
ortwice. How about:start = start or branch if start: ... -
reviewboard/hostingsvcs/bitbucket.py (Diff revision 2) Can you use keyword arguments for each parameter to
Commit? -
reviewboard/hostingsvcs/bitbucket.py (Diff revision 2) This is worth having a documented utility function for, as it's non-obvious what's going on.
-
reviewboard/hostingsvcs/bitbucket.py (Diff revision 2) 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). -
-
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.
Change Summary:
Address Christian's issues.
Description: |
|
|||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+135 -3) |
-
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 -
-
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
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