Adding smarter update notifications
Review Request #8519 — Created Nov. 3, 2016 and discarded
Information | |
---|---|
rswanson | |
Review Board | |
release-3.0.x | |
Reviewers | |
reviewboard, students | |
When viewing a review request, Review Board periodically checks
to see if there have been any updates to the review request.
Currently we only receive updates regarding the type of change
and who it was made by. The goal of this project is to improve
on the detail provided.
Added unit tests for backend code - all pass.
Manually verified API response locally.
Description | From | Last Updated |
---|---|---|
Col: 80 E501 line too long (84 > 79 characters) |
![]() |
|
undefined name 'DiffSet' |
![]() |
|
Col: 80 E501 line too long (97 > 79 characters) |
![]() |
|
The way you are currently doing this is going to build 7 lists. Each call to list() will create one … |
|
|
Col: 80 E501 line too long (97 > 79 characters) |
![]() |
|
This can throw if someone does ?timestamp=0 or some other invalid format. |
|
|
utc_timestamp will always be truthy. If we fail to parse the timestamp, we will get an exception. |
|
|
So there are 4 comment types (comments, general, file attachment, and screenshot). That means for every review we will be … |
|
|
We will probably want to get reviews and replies in two separate lists (may want to confer with christian on … |
|
|
This isn't being used anymore. |
|
|
Timestamps on a review are all set to the same value when published. This whole function wouldn't be doing anything … |
|
|
Make sure to follow the conventions for docstrings, specifically "Args" and "Returns". https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/ |
|
|
I don't think we should be adding new methods that just produce different variations of a query here. This query … |
|
|
We should always say "review request" instead of "request" (which tends to be the term for HTTP Request). |
|
|
This should not return DiffSets associated with review request drafts, i.e. review_request_draft=None. |
|
|
Same here as above. |
|
|
This should make sure that the reviews aren't drafts, i.e., public=True. |
|
|
timestamp doesn't need to be in quotes. Also can you change this to: data: { timestamp: this._lastUpdateTimestamp, }, |
|
|
Make sure this doesn't stay :) |
|
|
Instead of importing pytz, you should do from django.utils.timezone import utc. |
|
|
Single import statement, one name per line. |
|
|
This should be the WebAPI resource and not the model, i.e. [reviewboard.webapi.resources.diff.DiffResource] |
|
|
This shouldn't be a string. It should be a reference to the resource model. Same below. |
|
|
This should be the WebAPI resource and not the model, i.e. [reviewboard.webapi.resources.review.ReviewResource] |
|
|
This should be the WebAPI resource and not the model, i.e. [reviewboard.webapi.resources.base_comment.BaseCommentResource] (I think?) You will want to build the … |
|
|
"out-of-date" |
|
|
No blank line here. |
|
|
This comment isn't really clear. I think this is meant to be an example of the format, but if so, … |
|
|
Since you never use timestamp again, just reassign to timestamp. |
|
|
After you remove the pytz import you can convert to a aware time with (timezone.get_default_timezone() .localize(datetime .strptime(timestamp, '%Y-%m-%dT%H:%M:%SZ'))) |
CO Connor-Y | |
Comments should be proper sentences, periods and all. |
|
|
Just call this reviews. |
|
|
Blank line between code and new comments. |
|
|
This isn't too gross :). However, instead of iterating over this twice, we can iterate once: new_reviews = [] new_replies … |
|
|
Col: 80 E501 line too long (100 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (101 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |
|
This isn't too helpful to the caller. There should be suitable error information. However, we're also not actually working with … |
|
|
Docstring etc. I would rename new_reviews to just reviews. |
|
|
Can you make this a constant on the class? |
|
|
This needs to be review_id__in or review__pk__in. Also, one per line. |
|
|
from django.utils import six |
|
|
This file is missing a: from __future__ import unicode_literals Which must be at the top. |
|
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |
|
Missing a docstring. |
|
|
Blank line between these. |
|
|
Blank line between these. |
|
|
'INVALID_FORM_DATA' imported but unused |
![]() |
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |
|
Col: 9 E303 too many blank lines (2) |
![]() |
|
Col: 13 E116 unexpected indentation (comment) |
![]() |
|
Col: 13 E265 block comment should start with '# ' |
![]() |
|
Col: 13 E116 unexpected indentation (comment) |
![]() |
|
Col: 13 E116 unexpected indentation (comment) |
![]() |
|
Col: 63 E127 continuation line over-indented for visual indent |
![]() |
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |
|
undefined name 'DOES_NOT_EXIST' |
![]() |
|
Col: 80 E501 line too long (80 > 79 characters) |
![]() |
|
These can probably all be condensed into one test. |
|
|
undefined name 'DOES_NOT_EXIST' |
![]() |
|
Col: 80 E501 line too long (82 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (85 > 79 characters) |
![]() |
|
Col: 17 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 1 E302 expected 2 blank lines, found 1 |
![]() |
|
undefined name 'review_request' |
![]() |
|
undefined name 'DOES_NOT_EXIST' |
![]() |
|
Col: 80 E501 line too long (82 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (85 > 79 characters) |
![]() |
|
undefined name 'review' |
![]() |
|
undefined name 'review' |
![]() |
|
Col: 47 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 17 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 17 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 1 E302 expected 2 blank lines, found 1 |
![]() |
|
undefined name 'DOES_NOT_EXIST' |
![]() |
|
Col: 80 E501 line too long (82 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (85 > 79 characters) |
![]() |
|
Col: 47 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 17 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 17 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 1 E302 expected 2 blank lines, found 1 |
![]() |
|
Col: 80 E501 line too long (88 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (82 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (85 > 79 characters) |
![]() |
|
Col: 47 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 17 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 17 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 1 E302 expected 2 blank lines, found 1 |
![]() |
|
Col: 80 E501 line too long (88 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (82 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (85 > 79 characters) |
![]() |
|
Col: 47 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 17 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 17 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 1 E302 expected 2 blank lines, found 1 |
![]() |
|
'datetime' imported but unused |
![]() |
|
Col: 80 E501 line too long (82 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (85 > 79 characters) |
![]() |
|
Col: 17 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 37 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 42 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 27 E201 whitespace after '{' |
![]() |
|
Col: 32 E127 continuation line over-indented for visual indent |
![]() |
|
Col: 33 E201 whitespace after '{' |
![]() |
|
Col: 27 E201 whitespace after '{' |
![]() |
|
Col: 32 E127 continuation line over-indented for visual indent |
![]() |
|
Col: 33 E201 whitespace after '{' |
![]() |
|
Col: 1 E302 expected 2 blank lines, found 1 |
![]() |
|
'datetime' imported but unused |
![]() |
|
Col: 80 E501 line too long (82 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (85 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (81 > 79 characters) |
![]() |
|
Col: 17 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 26 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 17 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 37 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 42 E128 continuation line under-indented for visual indent |
![]() |
|
Col: 27 E201 whitespace after '{' |
![]() |
|
Col: 32 E127 continuation line over-indented for visual indent |
![]() |
|
Col: 33 E201 whitespace after '{' |
![]() |
|
Col: 27 E201 whitespace after '{' |
![]() |
|
Col: 32 E127 continuation line over-indented for visual indent |
![]() |
|
Col: 33 E201 whitespace after '{' |
![]() |
|
Col: 1 E302 expected 2 blank lines, found 1 |
![]() |
|
Undo this. |
|
|
This won't (or shouldn't, anyway) result with the timestamp saved to the review request in the DB. Instead of setting … |
|
|
This can eventually lead to fun to debug circular reference issues. Instead, when you want to refer to ReviewResource you … |
|
|
This should actually be: ['reviewboard.webapi.resources.diff.DiffResource'] |
|
|
Same goes for here with the appropriate resource |
|
|
Same goes for here with the appropriate resource |
|
|
Same goes for here with the appropriate resource |
|
|
Same goes for here with the appropriate resource |
|
|
Constant names should be ALL_CAPS. Also, can you format this as: COMMENT_TYPES = ( Comment, # ... ) |
|
|
Don't use exceptions like this for flow control. Instead: if not timestamp: # old behaviour return ... try: timestamp = … |
|
|
This documentation needs to go into the docs for the timestamp field. |
|
|
No blank line here. |
|
|
When the timestamp is provided and invalid, we want to give an INVALID_FORM_DATA response. |
|
|
missing docstring. |
|
|
Needs file docstring. |
|
|
Blank line between these. |
|
|
Blank line between these. |
|
|
Col: 80 E501 line too long (82 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (85 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (83 > 79 characters) |
![]() |
|
Col: 17 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 26 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 26 E126 continuation line over-indented for hanging indent |
![]() |
|
Docstring. |
|
|
Let's leave the comment, please. |
|
|
Even though the line would go over 80 columns, let's keep this in one piece. |
|
|
Col: 21 E128 continuation line under-indented for visual indent |
![]() |
|
This is only used once in an internal method, so let's just have it inline there. |
|
|
Col: 5 E124 closing bracket does not match visual indentation |
![]() |
|
We only use this in the one place, so let's avoid assigning the extra variable (or constructing the list): for … |
|
|
We shouldn't need to convert to list here. |
|
|
Keep this on one line, even though it's long. Should also be list of reviewboard.webapi.resource.review.ReviewResource |
|
|
Same here re list of .... This also doesn't need the parens. |
|
|
It looks like some of your other change has snuck in here. Do you have one branch dependent on the … |
|
|
Col: 80 E501 line too long (82 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (85 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (83 > 79 characters) |
![]() |
|
Col: 26 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 9 E124 closing bracket does not match visual indentation |
![]() |
|
Col: 26 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 9 E124 closing bracket does not match visual indentation |
![]() |
|
This line should be empty |
FI finaiized | |
Blank line in between here. |
|
|
Should be formatted as: Returns: unicode: The resulting absolute URL to teh item resource. |
|
|
Blank line between these. |
|
|
This should be the resource, e.g. ['reviewboard.webapi.resources.diff.DiffResource']. |
|
|
This should be the resource, e.g. ['reviewboard.webapi.resources.review.ReviewResource']. |
|
|
This should be the resource, e.g. ['reviewboard.webapi.resources.review_reply.ReviewReplyResource']. |
|
|
This should be the resource, e.g. ['reviewboard.webapi.resources.base_comment.BaseCommentResource']. |
|
|
This should be the resource, e.g. ['reviewboard.webapi.resources.change.ChangeResource']. |
|
|
You can refactor this to remove code duplication. try: timestamp = datetime.strptime(timestamp, ...).replace(tzinfo=utc) except ValueError: timestamp = None if timestamp … |
|
|
Col: 46 E127 continuation line over-indented for visual indent |
![]() |
|
Col: 46 E127 continuation line over-indented for visual indent |
![]() |
|
These keys are common between all responses, so you can further refactor the code to do: response_keys = { 'timestamp': … |
|
|
local variable 'comment_types' is assigned to but never used |
![]() |
|
Make this a class level constant, e.g., self.COMMENT_TYPES. |
|
|
Col: 9 E124 closing bracket does not match visual indentation |
![]() |
|
Col: 80 E501 line too long (82 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (85 > 79 characters) |
![]() |
|
Col: 80 E501 line too long (83 > 79 characters) |
![]() |
|
Col: 26 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 9 E124 closing bracket does not match visual indentation |
![]() |
|
Col: 26 E126 continuation line over-indented for hanging indent |
![]() |
|
Col: 9 E124 closing bracket does not match visual indentation |
![]() |
Status: Re-opened
Summary: |
|
|||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||
Diff: |
Revision 1 (+23 -6) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 2 (+84 -6) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_last_update.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_last_update.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/reviews/models/review_request.py (Diff revision 2) Col: 80 E501 line too long (84 > 79 characters)
Description: |
|
---|
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 3 (+122 -5) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_last_update.py reviewboard/reviews/models/review.py reviewboard/webapi/resources/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_last_update.py reviewboard/reviews/models/review.py reviewboard/webapi/resources/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 4 (+127 -5) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_last_update.py reviewboard/reviews/models/review.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_last_update.py reviewboard/reviews/models/review.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/reviews/models/review_request.py (Diff revision 4) Col: 80 E501 line too long (97 > 79 characters)
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 5 (+128 -5) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_last_update.py reviewboard/reviews/models/review.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_last_update.py reviewboard/reviews/models/review.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/reviews/models/review_request.py (Diff revision 5) Col: 80 E501 line too long (97 > 79 characters)
-
-
reviewboard/reviews/models/review.py (Diff revision 5) The way you are currently doing this is going to build 7 lists. Each call to
list()
will create one (so thats 4) and each addition will create a new intermediate list (and there are 3 of those). that is quite expensive in terms of memory allocation. Ideally we want to use one list and uselist.extend
:comment_types = ( self.comments, self.screenshot_comments, self.file_attachment_comments, self.general_comments, ) comments = [] for comment_type in comment_types: comments.extend(comment_type.filter(timestamp__gt=timestamp))
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 5) This can throw if someone does
?timestamp=0
or some other invalid format. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 5) utc_timestamp
will always be truthy. If we fail to parse the timestamp, we will get an exception. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 5) So there are 4 comment types (comments, general, file attachment, and screenshot). That means for every review we will be doing four queries, so we're going to be doing
4 * r
queries. We really don't want to be doing this.If we had a large review request, with say 100 reviews, someone could query the api with
?timestamp=1970-01-01T00:00:00Z
and get every review and we'd have to do 400 database queries. This can lead to denial of service attacks.We really want to do the minimum number of queries here, which will be four. Instead of going through each individual
review
, you'll have to query through each comment class, e.g.:review_pks = [review.pk for review in new_reviews] new_comments = [] comment_types = (Comment, FileAttachmentComment, GeneralComment, ScreenshotComment) for comment_cls in comment_types: new_comments.extend(comment_cls.objects.filter( review_id__in=review_pks, timestamp__gt=timestamp))
If we need to parse out comments to their respective reviews afterwards, we can post-process the dataset.
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 5) Replies are not comment updates. Replies are actually a
Review
object withbase_reply_to
having a non-null value. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 5) We will probably want to get reviews and replies in two separate lists (may want to confer with christian on that). However, you can do one query for each type of review + reply and then filter them out in python land.
Change Summary:
Addressing review comments. Broke up reviews and replies from being returned in the same list. Started developing unit tests in line with other API classes.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+184 -6) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/reviews/models/review.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/reviews/models/review.py reviewboard/reviews/models/review_request.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) Col: 80 E501 line too long (100 > 79 characters)
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) Col: 80 E501 line too long (101 > 79 characters)
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) Col: 80 E501 line too long (80 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 6) Col: 80 E501 line too long (80 > 79 characters)
-
-
-
reviewboard/reviews/models/review_request.py (Diff revision 6) This should not return
DiffSet
s associated with review request drafts, i.e.review_request_draft=None
. -
reviewboard/reviews/models/review_request.py (Diff revision 6) This should make sure that the reviews aren't drafts, i.e.,
public=True
. -
reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js (Diff revision 6) timestamp
doesn't need to be in quotes. Also can you change this to:data: { timestamp: this._lastUpdateTimestamp, },
-
reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js (Diff revision 6) Make sure this doesn't stay :)
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) Instead of importing
pytz
, you should dofrom django.utils.timezone import utc
. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) Single import statement, one name per line.
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) This should be the WebAPI resource and not the model, i.e.
[reviewboard.webapi.resources.diff.DiffResource]
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) This should be the WebAPI resource and not the model, i.e.
[reviewboard.webapi.resources.review.ReviewResource]
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) This should be the WebAPI resource and not the model, i.e.
[reviewboard.webapi.resources.base_comment.BaseCommentResource]
(I think?) You will want to build the docs and see if this one works. If so, theBaseCommentResource
will likely want to include links to all resource types in its class docstring.The docs you want to check are the
webapi/
docs, not the class documentation for the resource. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) Since you never use
timestamp
again, just reassign totimestamp
. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) Just call this
reviews
. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) This isn't too gross :). However, instead of iterating over this twice, we can iterate once:
new_reviews = [] new_replies = [] for review in reviews: if review.base_reply_to: new_replies.append(review) else: new_reviews.append(review)
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) Docstring etc.
I would rename
new_reviews
to justreviews
. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) review_ids
orreview_pks
are fine here. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) Can you make this a constant on the class?
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) This needs to be
review_id__in
orreview__pk__in
. Also, one per line. -
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 6) from django.utils import six
-
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) After you remove the pytz import you can convert to a aware time with
(timezone.get_default_timezone() .localize(datetime .strptime(timestamp, '%Y-%m-%dT%H:%M:%SZ')))
-
The overall results of the API look good. There's some additions in this change that don't need to be there, and some consistency things to work on. Good work so far, though!
-
reviewboard/reviews/models/review.py (Diff revision 6) Timestamps on a review are all set to the same value when published. This whole function wouldn't be doing anything other than
get_all_comments
. -
reviewboard/reviews/models/review.py (Diff revision 6) Make sure to follow the conventions for docstrings, specifically "Args" and "Returns".
https://www.reviewboard.org/docs/codebase/dev/docs/writing-codebase-docs/
-
reviewboard/reviews/models/review_request.py (Diff revision 6) I don't think we should be adding new methods that just produce different variations of a query here. This query is simple enough that it can and should be used by the caller.
The reason is that we don't want to establish precedent. We probably have a hundred or more distinct queries involving attributes of a
ReviewRequest
throughout the codebase, and if we had a method for each one in this class, the class would massively balloon up.So instead, let's get rid of this and just do this query directly where it needs to be called.
-
reviewboard/reviews/models/review_request.py (Diff revision 6) We should always say "review request" instead of "request" (which tends to be the term for HTTP Request).
-
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) This shouldn't be a string. It should be a reference to the resource model.
Same below.
-
-
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) This comment isn't really clear. I think this is meant to be an example of the format, but if so, the comment should state that. However, saying that it's a ISO8601 timestamp is sufficient.
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) Comments should be proper sentences, periods and all.
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) Blank line between code and new comments.
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 6) This isn't too helpful to the caller. There should be suitable error information.
However, we're also not actually working with form data, so the error is incorrect.
There's really a lot of code in this
try/except
. What part is expected to raise theValueError
? We should limit the code around that, and perhaps not error out but instead fall back to old behavior (not showing the new fields). -
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 6) This file is missing a:
from __future__ import unicode_literals
Which must be at the top.
-
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 6) Blank line between these.
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 6) Blank line between these.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 7 (+172 -7) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/test_review_request_last_update.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/test_review_request_last_update.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7) 'INVALID_FORM_DATA' imported but unused
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7) Col: 80 E501 line too long (80 > 79 characters)
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7) Col: 9 E303 too many blank lines (2)
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7) Col: 13 E116 unexpected indentation (comment)
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7) Col: 13 E265 block comment should start with '# '
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7) Col: 13 E116 unexpected indentation (comment)
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7) Col: 13 E116 unexpected indentation (comment)
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 7) Col: 63 E127 continuation line over-indented for visual indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 7) Col: 80 E501 line too long (80 > 79 characters)
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 8 (+171 -7) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/test_review_request_last_update.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/test_review_request_last_update.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 8) undefined name 'DOES_NOT_EXIST'
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 8) Col: 80 E501 line too long (80 > 79 characters)
-
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 8) These can probably all be condensed into one test.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 9 (+190 -7) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 9) undefined name 'DOES_NOT_EXIST'
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 9) Col: 80 E501 line too long (82 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 9) Col: 80 E501 line too long (85 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 9) Col: 17 E126 continuation line over-indented for hanging indent
-
-
Change Summary:
Trying to get the default get tests running on
test_review_request_last_update.py
- running in to some issues.
Testing Done: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 10 (+196 -7) |
Change Summary:
Built in get tests running.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 11 (+195 -7) |
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 12 (+206 -6) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 11) undefined name 'DOES_NOT_EXIST'
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 11) Col: 80 E501 line too long (82 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 11) Col: 80 E501 line too long (85 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 11) Col: 47 E128 continuation line under-indented for visual indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 11) Col: 17 E126 continuation line over-indented for hanging indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 11) Col: 17 E126 continuation line over-indented for hanging indent
-

-
Tool: Pyflakes Processed Files: reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: Pyflakes Processed Files: reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 10) undefined name 'DOES_NOT_EXIST'
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 10) Col: 80 E501 line too long (82 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 10) Col: 80 E501 line too long (85 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 10) undefined name 'review'
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 10) undefined name 'review'
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 10) Col: 47 E128 continuation line under-indented for visual indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 10) Col: 17 E126 continuation line over-indented for hanging indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 10) Col: 17 E126 continuation line over-indented for hanging indent
-
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 12) Col: 80 E501 line too long (88 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 12) Col: 80 E501 line too long (82 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 12) Col: 80 E501 line too long (85 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 12) Col: 47 E128 continuation line under-indented for visual indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 12) Col: 17 E126 continuation line over-indented for hanging indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 12) Col: 17 E126 continuation line over-indented for hanging indent
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 13 (+222 -6) |

-
Tool: Pyflakes Processed Files: reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 13) Col: 80 E501 line too long (88 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 13) Col: 80 E501 line too long (82 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 13) Col: 80 E501 line too long (85 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 13) Col: 47 E128 continuation line under-indented for visual indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 13) Col: 17 E126 continuation line over-indented for hanging indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 13) Col: 17 E126 continuation line over-indented for hanging indent
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 14 (+343 -8) |

-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14) 'datetime' imported but unused
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14) Col: 80 E501 line too long (82 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14) Col: 80 E501 line too long (85 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14) Col: 17 E126 continuation line over-indented for hanging indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14) Col: 37 E128 continuation line under-indented for visual indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14) Col: 42 E128 continuation line under-indented for visual indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14) Col: 27 E201 whitespace after '{'
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14) Col: 32 E127 continuation line over-indented for visual indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14) Col: 33 E201 whitespace after '{'
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14) Col: 27 E201 whitespace after '{'
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14) Col: 32 E127 continuation line over-indented for visual indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 14) Col: 33 E201 whitespace after '{'
-
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 15 (+353 -8) |

-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) 'datetime' imported but unused
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) Col: 80 E501 line too long (82 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) Col: 80 E501 line too long (85 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) Col: 80 E501 line too long (81 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) Col: 17 E126 continuation line over-indented for hanging indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) Col: 26 E126 continuation line over-indented for hanging indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) Col: 17 E126 continuation line over-indented for hanging indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) Col: 37 E128 continuation line under-indented for visual indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) Col: 42 E128 continuation line under-indented for visual indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) Col: 27 E201 whitespace after '{'
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) Col: 32 E127 continuation line over-indented for visual indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) Col: 33 E201 whitespace after '{'
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) Col: 27 E201 whitespace after '{'
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) Col: 32 E127 continuation line over-indented for visual indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 15) Col: 33 E201 whitespace after '{'
-
Change Summary:
Resolving unit test comparator function.
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 16 (+359 -8) |

-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16) Col: 80 E501 line too long (82 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16) Col: 80 E501 line too long (85 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16) Col: 80 E501 line too long (83 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16) Col: 17 E126 continuation line over-indented for hanging indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16) Col: 26 E126 continuation line over-indented for hanging indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16) Col: 26 E126 continuation line over-indented for hanging indent
-
-
-
reviewboard/testing/testcase.py (Diff revision 16) This won't (or shouldn't, anyway) result with the timestamp saved to the review request in the DB. Instead of setting review.timestamp, and calling
save()
(which is whatpublish()
), you will have to do:Review.objects.filter(pk=review.pk).update(timestamp=timestamp)
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16) This can eventually lead to fun to debug circular reference issues. Instead, when you want to refer to
ReviewResource
you can do:resources.review
. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16) This should actually be:
['reviewboard.webapi.resources.diff.DiffResource']
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16) Same goes for here with the appropriate resource
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16) Same goes for here with the appropriate resource
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16) Same goes for here with the appropriate resource
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16) Same goes for here with the appropriate resource
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16) Constant names should be
ALL_CAPS
.Also, can you format this as:
COMMENT_TYPES = ( Comment, # ... )
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16) Don't use exceptions like this for flow control.
Instead:
if not timestamp: # old behaviour return ... try: timestamp = datetime.strptime(...) except ValueError: return INVALID_FORM_DATA, { fields: { 'timestamp': [ 'error here', ], }, } # continue calculating advanced response.
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16) This documentation needs to go into the docs for the timestamp field.
-
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16) When the timestamp is provided and invalid, we want to give an
INVALID_FORM_DATA
response. -
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16) Needs file docstring.
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16) Blank line between these.
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 16) Blank line between these.
-
-
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 16) is the timestamp param passed into this function the curent timestamp or is when the last update was made (which I assumed) and how is it differnt from the server_timestamp?
Commit: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 17 (+414 -9) |

-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/root.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 17) Col: 21 E128 continuation line under-indented for visual indent
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 17) Col: 5 E124 closing bracket does not match visual indentation
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 17) Col: 80 E501 line too long (82 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 17) Col: 80 E501 line too long (85 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 17) Col: 80 E501 line too long (83 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 17) Col: 26 E126 continuation line over-indented for hanging indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 17) Col: 9 E124 closing bracket does not match visual indentation
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 17) Col: 26 E126 continuation line over-indented for hanging indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 17) Col: 9 E124 closing bracket does not match visual indentation
-
-
reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js (Diff revision 17) Let's leave the comment, please.
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 17) Even though the line would go over 80 columns, let's keep this in one piece.
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 17) This is only used once in an internal method, so let's just have it inline there.
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 17) We only use this in the one place, so let's avoid assigning the extra variable (or constructing the list):
for review in Review.objects.filter( review_request=review_request, timestamp__gt=timestamp, public=True): if review.base_reply_to: ...
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 17) We shouldn't need to convert to
list
here. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 17) Keep this on one line, even though it's long.
Should also be
list of reviewboard.webapi.resource.review.ReviewResource
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 17) Same here re
list of ...
. This also doesn't need the parens. -
reviewboard/webapi/resources/root.py (Diff revision 17) It looks like some of your other change has snuck in here. Do you have one branch dependent on the other?
-
-
reviewboard/webapi/tests/urls.py (Diff revision 17) Should be formatted as:
Returns: unicode: The resulting absolute URL to teh item resource.
Branch: |
|
||||
---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 18 (+414 -7) |

-
Tool: Pyflakes Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js Tool: PEP8 Style Checker Processed Files: reviewboard/testing/testcase.py reviewboard/webapi/tests/test_review_request_last_update.py reviewboard/webapi/resources/review_request_last_update.py reviewboard/webapi/tests/urls.py reviewboard/webapi/tests/mimetypes.py Ignored Files: reviewboard/static/rb/js/resources/models/reviewRequestModel.es6.js
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18) Col: 46 E127 continuation line over-indented for visual indent
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18) Col: 46 E127 continuation line over-indented for visual indent
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18) local variable 'comment_types' is assigned to but never used
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18) Col: 9 E124 closing bracket does not match visual indentation
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 18) Col: 80 E501 line too long (82 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 18) Col: 80 E501 line too long (85 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 18) Col: 80 E501 line too long (83 > 79 characters)
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 18) Col: 26 E126 continuation line over-indented for hanging indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 18) Col: 9 E124 closing bracket does not match visual indentation
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 18) Col: 26 E126 continuation line over-indented for hanging indent
-
reviewboard/webapi/tests/test_review_request_last_update.py (Diff revision 18) Col: 9 E124 closing bracket does not match visual indentation
-
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18) Blank line between these.
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18) This should be the resource, e.g.
['reviewboard.webapi.resources.diff.DiffResource']
. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18) This should be the resource, e.g.
['reviewboard.webapi.resources.review.ReviewResource']
. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18) This should be the resource, e.g.
['reviewboard.webapi.resources.review_reply.ReviewReplyResource']
. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18) This should be the resource, e.g.
['reviewboard.webapi.resources.base_comment.BaseCommentResource']
. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18) This should be the resource, e.g.
['reviewboard.webapi.resources.change.ChangeResource']
. -
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18) You can refactor this to remove code duplication.
try: timestamp = datetime.strptime(timestamp, ...).replace(tzinfo=utc) except ValueError: timestamp = None if timestamp is None: # Old behaviour. return 200, { ... }, { 'ETag': etag, } # New behaviour ...
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18) These keys are common between all responses, so you can further refactor the code to do:
response_keys = { 'timestamp': server_timestamp, 'user': user, 'summary': summary, 'type': update_type, } # try/except to check for timestamp ... if timestamp is None: # Calculate new_diffsets, etc. response_keys.update({ 'diffset_update': new_diffsets, 'review_updates': new_reviews, 'reply_updates': new_replies, 'comment_updates': new_comments, 'field_updates': field_updates, }) return 200, { self.item_result_key: response_keys, }, { 'ETag': etag, }
This eliminates the multiple returns and makes it easier to follow.
Additionally, can you ensure the keys are listed in alphabetical order when being inserted?
-
reviewboard/webapi/resources/review_request_last_update.py (Diff revision 18) Make this a class level constant, e.g.,
self.COMMENT_TYPES
.