-
-
reviewboard/extensions/tests.py (Diff revision 1) We want two blank lines. You shouldn't alter lines unrelated to your change.
-
-
-
reviewboard/extensions/tests.py (Diff revision 1) Missing a docstring.
I don't understand the point of splitting this into two functions?
-
reviewboard/reviews/models/review_request.py (Diff revision 1) This is defeating the purpose of the sandboxing work, since it's still crashing the program if there's an error. The whole point is to proceed so that normal operations can continue.
Sandboxed ReviewRequestApprovalHook and added test cases
Review Request #5677 — Created April 5, 2014 and submitted
Information | |
---|---|
edwinzg | |
Review Board | |
master | |
Reviewers | |
reviewboard, students | |
Sandboxed ReviewRequestApprovalHook and added test cases
Added Sandbox Extension and test hook.
Unit testing, added to test suites in reviewboard/extensions/tests.py
Ran unit tests with and without the sandboxing
Description | From | Last Updated |
---|---|---|
We want two blank lines. You shouldn't alter lines unrelated to your change. |
|
|
This is missing a docstring. |
|
|
""" at the beginning. |
|
|
Missing a docstring. I don't understand the point of splitting this into two functions? |
|
|
This is defeating the purpose of the sandboxing work, since it's still crashing the program if there's an error. The … |
|
|
Please sort these in alphabetical order (errors before manager). |
|
|
Please keep the things that are imported in alphabetical order. |
|
|
I think you should subclass ReviewRequestApprovalHook and implement your own custom is_approved method (which raises some error), rather than relying … |
|
|
This logging statement should include the extension name/id, as well as the contents of the exception that was raised. |
|
|
We don't want to raise any errors here, we just want to log and continue with the other hooks. |
|
|
This should be updated now, since is_approved is implemented. |
|
|
The logging methods will format the string with any args that they get, so it's better to pass them in … |
|
|
This should be "ReviewRequestApprovalHook" |
|
|
The "throws an error" should be aligned with """ and not "Testing" |
|
Description: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+33 -2) |
-
-
reviewboard/extensions/tests.py (Diff revision 2) Please sort these in alphabetical order (errors before manager).
-
reviewboard/extensions/tests.py (Diff revision 2) Please keep the things that are imported in alphabetical order.
-
reviewboard/extensions/tests.py (Diff revision 2) I think you should subclass ReviewRequestApprovalHook and implement your own custom is_approved method (which raises some error), rather than relying on the fact that the default implementation currently raises NotImplementedError.
ReviewRequestApprovalHook also does not take an
entries
parameter for its__init__
method, so you can just get rid of the whole entry part. -
reviewboard/reviews/models/review_request.py (Diff revision 2) This logging statement should include the extension name/id, as well as the contents of the exception that was raised.
-
reviewboard/reviews/models/review_request.py (Diff revision 2) We don't want to raise any errors here, we just want to log and continue with the other hooks.
Description: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+55 -13) |
-
-
reviewboard/extensions/tests.py (Diff revision 3) This should be updated now, since is_approved is implemented.
-
reviewboard/reviews/models/review_request.py (Diff revision 3) The logging methods will format the string with any args that they get, so it's better to pass them in as arguments instead of doing the format yourself (because then there will be two format operations). I'd also like to switch the format string around a bit--personally I think this is a little weird:
Error calling function is_approved: Permission Denide: MyExtension
Thus, this should look something like this:
logging.error('Error when running ReviewRequestApprovalHook.' 'is_approved in extension "%s": %s', extension.metadata['Name'], e, exc_info=1)
Summary: |
|
||||
---|---|---|---|---|---|
Testing Done: |
|
||||
Diff: |
Revision 4 (+56 -13) |
-
One last comment, and then this looks good!
-
-
-
reviewboard/extensions/tests.py (Diff revision 4) The "throws an error" should be aligned with
"""
and not "Testing"
Testing Done: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+56 -13) |