-
-
reviewboard/testing/scmtool.py (Diff revision 1) Col: 17 E127 continuation line over-indented for visual indent
-
reviewboard/webapi/resources/review_request.py (Diff revision 1) Col: 31 E127 continuation line over-indented for visual indent
Updating changenum with PUT call to ReviewRequest
Review Request #6774 — Created Jan. 15, 2015 and submitted
Making a PUT request to the webapi for ReviewRequest such as
curl http://reviewboard.example.com/api/review-requests/8/ -X PUT -d changenum=1234for a perforce review may not save the commit_id properly. This lead to two modifications made to the PUT command regarding changenum. Conversion of changenum to six.text_type in the event when changenum is entered as an integer for variable type conservation. The other is to change the save logic, updating the fields that have been changed.
Two Test cases were made
-
Tests the initialization of ReviewRequest with changenum, and checks if the instance has the proper changenum and an empty
commit_id. -
Tests the PUT call with changenum, and ensures the payload and the ReviewRequest saved have their changenum and commit_id changed to match up. This includes the implementation for TestTool.get_changeset was made, for scmtools that support pending changesets.
| Description | From | Last Updated |
|---|---|---|
|
Col: 17 E127 continuation line over-indented for visual indent |
 reviewbot
|
|
|
Col: 31 E127 continuation line over-indented for visual indent |
reviewbot
|
|
|
What happens if you have both extra_fields and the changenum was updated? I'm also not a big fan of adding … |
 david
|
|
|
Col: 25 W292 no newline at end of file |
reviewbot
|
|
|
Col: 45 E502 the backslash is redundant between brackets |
reviewbot
|
VT
Change Summary:
Uploaded changes to address indentation issue.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+56 -3) |
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/testing/scmtool.py reviewboard/reviews/tests.py reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/testing/scmtool.py reviewboard/reviews/tests.py reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py
-
-
reviewboard/webapi/resources/review_request.py (Diff revision 2) What happens if you have both extra_fields and the changenum was updated?
I'm also not a big fan of adding this new state variable.
How about we define an
changed_fieldslist, in which we can put 'extra_data' and 'commit_id'. Then we'd replace this with something like:... (define list and add in commit_id above) if extra_fields: self.import_extra_data(review_request, review_request.extra_data, extra_fields) changed_fields.append('extra_data') if changed_fields: review_request.save(update_fields=changed_fields)
VT
Change Summary:
Made adjustments as suggested.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+58 -4) |
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/testing/scmtool.py reviewboard/reviews/tests.py reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/testing/scmtool.py reviewboard/reviews/tests.py reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py
-
<p>The code looks good now.</p>
<p>Can you make a change to your description to talk about the bugfix itself first, and the details of the tests second?</p>
VT
Change Summary:
Updated the description, and moved the test cases to the "Testing Done" section.
Description: |
|
|||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
-
I just applied this patch but it broke several unit tests. I think the issue is your changes to TestTool. It might make sense to extract those into a subclass and use it only for your newly-added tests.
VT
Change Summary:
Updated to separate the TestTool for SCM Tools to be two different separate classes, the base one (the normal one used before implementation) and a new one, TestToolSupportsPendingChangeSets, an SCM Test tool that supports pending change sets.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+63 -5) |
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/testing/scmtool.py reviewboard/reviews/tests.py reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/testing/scmtool.py reviewboard/reviews/tests.py reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py -
-
reviewboard/testing/testcase.py (Diff revision 4) Col: 45 E502 the backslash is redundant between brackets
VT
Change Summary:
Adjusted for ReviewBot styling
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 5 (+63 -5) |
-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/testing/scmtool.py reviewboard/reviews/tests.py reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/testing/scmtool.py reviewboard/reviews/tests.py reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/testing/scmtool.py reviewboard/reviews/tests.py reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/testing/scmtool.py reviewboard/reviews/tests.py reviewboard/webapi/resources/review_request.py reviewboard/webapi/tests/test_review_request.py