-
-
-
-
reviewboard/hostingsvcs/github.py (Diff revision 1) Col: 80 E501 line too long (117 > 79 characters)
Multi-commit Pull Request Diff Support
Review Request #8547 — Created Nov. 19, 2016 and updated
| Information | |
|---|---|
| finaiized | |
| Review Board | |
| dvcs | |
|
|
|
| cc31765... | |
| Reviewers | |
| reviewboard, students | |
Reviewboard 4 introduces the concept of a
DiffCommit, where a list of them can be used to represent a series of commits. This is especially helpful for pull requests, as they are often created and updated with multiple commits at a time.All the commits in the pull request when the review request is created will be added to a revision. Further updates to the pull request will cause all the commits (not just the commits since then) to be added to the next revision, and so on.
This integrates
DiffCommitwith the current pull request implementation; the aim is to support pull requests created with multiple commits.
Added unit tests
- to verify the pull request hook is called, and only works with pull_request events and the right signature
- to verify pull requests are created and updated when a pull request is created
- to verify the pull request's status starts off with 'pending', is 'error' when there's open issues, and 'success' when approvedManual testing
- Created a pull request on a repository linked to ReviewBoard through webhooks
- Verified the review request was created with 'pending' status
- Verified that opening an issue on the review request sets the 'error' status on the pull request
- Verified that having no open issue and adding a 'Ship it!' sets the 'success' status on the pull request
- Verified that pull requests with multiple commits get put into a singleDiffSetand get rendered together.
| Description | From | Last Updated |
|---|---|---|
|
'datetime' imported but unused |
 reviewbot
|
|
|
Col: 80 E501 line too long (96 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (117 > 79 characters) |
reviewbot
|
|
|
'datetime' imported but unused |
reviewbot
|
|
|
Col: 80 E501 line too long (85 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (91 > 79 characters) |
reviewbot
|
|
|
This should be non-None if len(d['parents']) > 1. |
 brennie
|
|
|
'datetime' imported but unused |
reviewbot
|
|
|
'ipdb' imported but unused |
reviewbot
|
|
|
Col: 80 E501 line too long (85 > 79 characters) |
reviewbot
|
|
|
Col: 80 E501 line too long (91 > 79 characters) |
reviewbot
|
|
|
'DiffSet' imported but unused |
reviewbot
|
|
|
Alphabetical order. |
brennie
|
|
|
Missing 'Args' in docstring. Also this needs to go into deeper detail about what it actually does i.e. publishes the … |
brennie
|
|
|
If these are constants that GitHub expects, make them constants on the class. e.g. PR_STATUS_SUCCESS etc. |
brennie
|
|
|
Can you outline what the status means? |
brennie
|
|
|
"The repository that the commit belongs to." |
brennie
|
|
|
Missing period. "The full ID of the commit for which to set the status." |
brennie
|
|
|
Paragraphs should have a blank line between them. "The status to set for the commit." |
brennie
|
|
|
When these are constants you should outline them with, e.g. This must be one of the following values: * :py:attr:`CONSTANT_ONE` … |
brennie
|
|
|
"The description ..." The defaults of params show up in documentation. |
brennie
|
|
|
"unicode, optional" |
brennie
|
|
|
The defaults of params show up in documentation."The HTTP(S) URL..." |
brennie
|
|
|
No blank line here. |
brennie
|
|
|
This needs to be a constant on the class. |
brennie
|
|
|
Missing "args" and "returns" sections. This must be written in the imperitive mood, i.e., "Create or update ...." |
brennie
|
|
|
Return a 400 bad request and log that the user doesn't exist? |
brennie
|
|
|
This should probably be a constant in the siteconfig |
brennie
|
|
|
Since this can fail, we should surround this with a try/except. In the event of a failure, we should log … |
brennie
|
|
|
.split('=', 1) will ensure we only split on the first = which is what we're going to want to do … |
brennie
|
|
|
We should log this. |
brennie
|
|
|
"Create" |
brennie
|
|
|
"The repository that the review request is associated with." |
brennie
|
|
|
dict |
brennie
|
|
|
django.http.HttpRequest, optional |
brennie
|
|
|
Regarding my comment that this should throw SCMError, this must document that fact: Raises: reviewboard.scmtools.errors.SCMError: Raised when .... |
brennie
|
|
|
No blank line here. |
brennie
|
|
|
an SCMError is probably more appropriate here. |
brennie
|
|
|
Ths should pass the exception up the call stack and should log it in the method that calls this. |
brennie
|
|
|
In comments from mentors, I've been asked to provide the fully qualified path to the class type in documentation. |
RS rswanson | |
|
Super small nit - but this line should be aligned with the one above it. |
RS rswanson | |
|
Blank line between these. |
brennie
|
|
|
This should probably render a link to the external pull request. |
brennie
|
|
|
This is going to do up to three database queries: get the review request; get the repository; and get the … |
brennie
|
|
|
No blank line here. |
brennie
|
|
|
See previous comment re: number of queries. |
brennie
|
|
|
Single quotes. Capitilization. |
brennie
|
|
|
Use the imperitive mood ("return") and also missing "Returns" |
brennie
|
|
|
No blank line. |
brennie
|
|
|
Missing docstring. |
brennie
|
|
|
Can you reformat this as: unique_together = ( (..., ...), (..., ...), ... ) |
brennie
|
|
|
Col: 80 E501 line too long (80 > 79 characters) |
reviewbot
|
FI
Change Summary:
- Support multi-commit diffs when the PR is original created
- Test support for updating diffs when the PR is updated
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+326 -20) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/evolutions/pull_request.py reviewboard/reviews/models/base_comment.py reviewboard/reviews/evolutions/__init__.py reviewboard/diffviewer/diffutils.py reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review.py reviewboard/hostingsvcs/service.py reviewboard/hostingsvcs/github.py reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/evolutions/pull_request.py reviewboard/reviews/models/base_comment.py reviewboard/reviews/evolutions/__init__.py reviewboard/diffviewer/diffutils.py reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review.py reviewboard/hostingsvcs/service.py reviewboard/hostingsvcs/github.py reviewboard/reviews/models/review_request.py -
-
-
-
-
reviewboard/hostingsvcs/github.py (Diff revision 2) This should be non-None if
len(d['parents']) > 1.
FI
Change Summary:
- Rebased on changes made in #8463
- Fixed passing in the wrong request and revision into DiffCommit
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+316 -20) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/evolutions/pull_request.py reviewboard/reviews/models/base_comment.py reviewboard/reviews/evolutions/__init__.py reviewboard/diffviewer/diffutils.py reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review.py reviewboard/hostingsvcs/service.py reviewboard/hostingsvcs/github.py reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/evolutions/pull_request.py reviewboard/reviews/models/base_comment.py reviewboard/reviews/evolutions/__init__.py reviewboard/diffviewer/diffutils.py reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review.py reviewboard/hostingsvcs/service.py reviewboard/hostingsvcs/github.py reviewboard/reviews/models/review_request.py -
-
-
-
FI
Change Summary:
- Correctly support revisions for multi-commit diffs
- Code cleanup and refactoring
- Throw error when merge commits are encountered. Merge commits are currently not supported by throughout Reviewboard.
Description: |
|
||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||
Diff: |
Revision 4 (+306 -19) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/evolutions/pull_request.py reviewboard/reviews/models/base_comment.py reviewboard/reviews/evolutions/__init__.py reviewboard/diffviewer/diffutils.py reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review.py reviewboard/hostingsvcs/service.py reviewboard/hostingsvcs/github.py reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/evolutions/pull_request.py reviewboard/reviews/models/base_comment.py reviewboard/reviews/evolutions/__init__.py reviewboard/diffviewer/diffutils.py reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review.py reviewboard/hostingsvcs/service.py reviewboard/hostingsvcs/github.py reviewboard/reviews/models/review_request.py -
FI
Change Summary:
Remove unusued import.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+306 -19) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/evolutions/pull_request.py reviewboard/reviews/models/base_comment.py reviewboard/reviews/evolutions/__init__.py reviewboard/diffviewer/diffutils.py reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review.py reviewboard/hostingsvcs/service.py reviewboard/hostingsvcs/github.py reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/evolutions/pull_request.py reviewboard/reviews/models/base_comment.py reviewboard/reviews/evolutions/__init__.py reviewboard/diffviewer/diffutils.py reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review.py reviewboard/hostingsvcs/service.py reviewboard/hostingsvcs/github.py reviewboard/reviews/models/review_request.py
-
-
-
reviewboard/hostingsvcs/github.py (Diff revision 5) Missing 'Args' in docstring. Also this needs to go into deeper detail about what it actually does i.e. publishes the status to github.
-
reviewboard/hostingsvcs/github.py (Diff revision 5) If these are constants that GitHub expects, make them constants on the class. e.g.
PR_STATUS_SUCCESSetc. -
-
-
reviewboard/hostingsvcs/github.py (Diff revision 5) Missing period.
"The full ID of the commit for which to set the status."
-
reviewboard/hostingsvcs/github.py (Diff revision 5) Paragraphs should have a blank line between them.
"The status to set for the commit."
-
reviewboard/hostingsvcs/github.py (Diff revision 5) When these are constants you should outline them with, e.g.
This must be one of the following values: * :py:attr:`CONSTANT_ONE` * :py:attr:`CONSTANT_TWO`That will render as a list with links to the relevant constants in the documentation.
-
reviewboard/hostingsvcs/github.py (Diff revision 5) "The description ..."
The defaults of params show up in documentation.
-
-
reviewboard/hostingsvcs/github.py (Diff revision 5) The defaults of params show up in documentation."The HTTP(S) URL..."
-
-
-
reviewboard/hostingsvcs/github.py (Diff revision 5) Missing "args" and "returns" sections.
This must be written in the imperitive mood, i.e., "Create or update ...."
-
reviewboard/hostingsvcs/github.py (Diff revision 5) Return a 400 bad request and log that the user doesn't exist?
-
reviewboard/hostingsvcs/github.py (Diff revision 5) This should probably be a constant in the siteconfig
-
reviewboard/hostingsvcs/github.py (Diff revision 5) Since this can fail, we should surround this with a
try/except. In the event of a failure, we should log the error and return a 400. -
reviewboard/hostingsvcs/github.py (Diff revision 5) .split('=', 1)will ensure we only split on the first=which is what we're going to want to do here. This will simplify the logic of the following conditional. -
-
-
reviewboard/hostingsvcs/github.py (Diff revision 5) "The repository that the review request is associated with."
-
-
-
reviewboard/hostingsvcs/github.py (Diff revision 5) Regarding my comment that this should throw
SCMError, this must document that fact:Raises: reviewboard.scmtools.errors.SCMError: Raised when .... -
-
-
reviewboard/hostingsvcs/github.py (Diff revision 5) Ths should pass the exception up the call stack and should log it in the method that calls this.
-
-
reviewboard/reviews/builtin_fields.py (Diff revision 5) This should probably render a link to the external pull request.
-
reviewboard/reviews/models/base_comment.py (Diff revision 5) This is going to do up to three database queries:
- get the review request;
- get the repository; and
- get the hosting service account.
This will make this method very slow. To counteract this, we can just query for that hosting service account and review_request and from there get the hosting service as follows:
review_request = review.review_request hosting_svc_account = HostingServiceAccount.objects.get(repository__review_requests__reviews=review) hosting_svc_account.service.update_pull_request(review_request)Then you can use
review_requestin the update call below as well. -
-
-
-
reviewboard/reviews/models/review_request.py (Diff revision 5) Use the imperitive mood ("return") and also missing "Returns"
-
-
-
reviewboard/reviews/models/review_request.py (Diff revision 5) Can you reformat this as:
unique_together = ( (..., ...), (..., ...), ... )
RS
-
Looks really good! Had a few musings below.
-
reviewboard/hostingsvcs/service.py (Diff revision 5) In comments from mentors, I've been asked to provide the fully qualified path to the class type in documentation.
-
reviewboard/reviews/builtin_fields.py (Diff revision 5) Super small nit - but this line should be aligned with the one above it.
FI
Change Summary:
Changes based on reviews
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+658 -23) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/evolutions/pull_request.py reviewboard/reviews/models/base_comment.py reviewboard/reviews/evolutions/__init__.py reviewboard/hostingsvcs/tests/test_github.py reviewboard/diffviewer/diffutils.py reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review.py reviewboard/hostingsvcs/service.py reviewboard/hostingsvcs/github.py reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/evolutions/pull_request.py reviewboard/reviews/models/base_comment.py reviewboard/reviews/evolutions/__init__.py reviewboard/hostingsvcs/tests/test_github.py reviewboard/diffviewer/diffutils.py reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review.py reviewboard/hostingsvcs/service.py reviewboard/hostingsvcs/github.py reviewboard/reviews/models/review_request.py -
FI
Change Summary:
- Fix overrunning line
- Removed WIP tag and added testing info
Summary: |
|
|||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||
Diff: |
Revision 7 (+659 -23) |
-
Tool: Pyflakes Processed Files: reviewboard/reviews/evolutions/pull_request.py reviewboard/reviews/models/base_comment.py reviewboard/reviews/evolutions/__init__.py reviewboard/hostingsvcs/tests/test_github.py reviewboard/diffviewer/diffutils.py reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review.py reviewboard/hostingsvcs/service.py reviewboard/hostingsvcs/github.py reviewboard/reviews/models/review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/evolutions/pull_request.py reviewboard/reviews/models/base_comment.py reviewboard/reviews/evolutions/__init__.py reviewboard/hostingsvcs/tests/test_github.py reviewboard/diffviewer/diffutils.py reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review.py reviewboard/hostingsvcs/service.py reviewboard/hostingsvcs/github.py reviewboard/reviews/models/review_request.py
Tool: Pyflakes Processed Files: reviewboard/reviews/evolutions/pull_request.py reviewboard/reviews/models/base_comment.py reviewboard/reviews/evolutions/__init__.py reviewboard/reviews/models/review_request.py reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review.py reviewboard/hostingsvcs/service.py reviewboard/hostingsvcs/github.py Tool: PEP8 Style Checker Processed Files: reviewboard/reviews/evolutions/pull_request.py reviewboard/reviews/models/base_comment.py reviewboard/reviews/evolutions/__init__.py reviewboard/reviews/models/review_request.py reviewboard/reviews/builtin_fields.py reviewboard/reviews/models/review.py reviewboard/hostingsvcs/service.py reviewboard/hostingsvcs/github.py