flake8
-
extension/reviewbotext/resources.py (Diff revision 1) Show all issues -
-
Review Request #10095 — Created July 19, 2018 and discarded
Adding the ability for bots to make general comments.
Similar to how a tool can post diff comments on a file, they can now also post general comments on the review.
This also paves the way for tools to post file and screenshot comments (but does not add this functionality)
Was able to post an automated review with/without general comments, opening issues on some comments and not on others.
Also posted an automated review with 35 general comments and 1 diff comment -> only 30 got posted, all of which were the general comments.
Likewise posted an automated review with 15 general comments and 17 diff comments -> only 30 (total) got posted, 15 general and 15 diff.
Additionally, tried using an updated Review Board server with a worker without the changes -> No errors, everything works as expected.
Description | From | Last Updated |
---|---|---|
E302 expected 2 blank lines, found 1 |
![]() |
|
E501 line too long (81 > 79 characters) |
![]() |
|
E722 do not use bare except' |
![]() |
|
This also needs to clear out self.comments |
|
|
Can we call this InvalidFormDataError? |
|
|
Docstrings should be a single-line summary followed by zero or more paragraphs. |
|
|
Blank line between the docstring and the methods. |
|
|
This needs a docstring. |
|
|
Shouldn't this be return INVALID_FORM_DATA, e.data? |
|
|
Formatting here can be improved (or at least made more consistent with other code): raise InvalidFormData({ 'fields': { comment_type: 'Malformed … |
|
|
Same here: raise InvalidFormData({ 'fields': { comment_type: ('Element missing keys "%s"' % missing_keys), }, }) |
|
|
Can we move this code into the try/catch above? That way we only have one place that's returning INVALID_FORM_DATA. We … |
|
|
It would be slightly cleaner to have these in the try/catch above where we assign comment['filediff'] |
|
|
This needs a docstring |
|
|
Add a trailing comma |
|
|
Can we put parens around the value here? comment_type: ('Element missing keys "%s"' % missing_keys), |
|
|
Add a trailing comma |
|
extension/reviewbotext/resources.py (Diff revision 1) |
---|
extension/reviewbotext/resources.py (Diff revision 1) |
---|
This wasn't being done before, and invalid JSON would result in
APIError 500
.
Appeasing the 8 gods of Flake.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+107 -37) |
extension/reviewbotext/resources.py (Diff revision 2) |
---|
Docstrings should be a single-line summary followed by zero or more paragraphs.
extension/reviewbotext/resources.py (Diff revision 2) |
---|
Blank line between the docstring and the methods.
extension/reviewbotext/resources.py (Diff revision 2) |
---|
Shouldn't this be
return INVALID_FORM_DATA, e.data
?
extension/reviewbotext/resources.py (Diff revision 2) |
---|
Formatting here can be improved (or at least made more consistent with other code):
raise InvalidFormData({ 'fields': { comment_type: 'Malformed JSON.', }, })
extension/reviewbotext/resources.py (Diff revision 2) |
---|
Same here:
raise InvalidFormData({ 'fields': { comment_type: ('Element missing keys "%s"' % missing_keys), }, })
Integrated David's feedback + more testing
Testing Done: |
|
||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Commit: |
|
||||||||||||||||||
Diff: |
Revision 3 (+118 -37) |
extension/reviewbotext/resources.py (Diff revision 3) |
---|
Can we move this code into the try/catch above? That way we only have one place that's returning INVALID_FORM_DATA.
We could also do all of the normalization before creating the Review object, which would eliminate the need for the TODO comment since we wouldn't have created anything in the db.
extension/reviewbotext/resources.py (Diff revision 3) |
---|
It would be slightly cleaner to have these in the try/catch above where we assign
comment['filediff']
extension/reviewbotext/resources.py (Diff revision 3) |
---|
Can we put parens around the value here?
comment_type: ('Element missing keys "%s"' % missing_keys),