This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/webapi/resources.py
    reviewboard/reviews/models.py
    reviewboard/reviews/tests.py
    reviewboard/webapi/tests.py
    reviewboard/reviews/managers.py
    reviewboard/reviews/datagrids.py
    reviewboard/reviews/evolutions/issue_status_count.py
  Ignored Files:
    reviewboard/templates/reviews/review_detail.html
    reviewboard/static/rb/css/common.less
    reviewboard/reviews/fixtures/test_reviewrequests.json
    reviewboard/static/rb/css/reviews.less

Col: 25
 E127 continuation line over-indented for visual indent

Col: 25
 E127 continuation line over-indented for visual indent

Col: 30
 E127 continuation line over-indented for visual indent

Col: 21
 E127 continuation line over-indented for visual indent

Col: 16
 E221 multiple spaces before operator

Col: 11
 E221 multiple spaces before operator

Col: 80
 E501 line too long (97 > 79 characters)

Nitpicks:

- Would be nice if issues had a subtle gradient background like 'ship it'.
- Would be nice if the status bubble didn't have so much extra empty space to the right. (For that matter, should the number be right-justified?)

...but I'd rather have it land without those tweaks than not at all :-).

Thanks for working on this!

as discussed on issue #2723
http://code.google.com/p/reviewboard/issues/detail?id=2723#c14

to prevent counting issue_status=O+ opened_issue=0 bug #2984
https://code.google.com/p/reviewboard/issues/detail?id=2984

I'd propose to prefix test by "comment.issue_opened and"
=>
                   if comment.issue_opened and
                      comment.issue_status in issue_status)

to prevent counting issue_status=O+ opened_issue=0 bug #2984
https://code.google.com/p/reviewboard/issues/detail?id=2984

I'd propose to prefix test by "comment.issue_opened and"
=>
                   if comment.issue_opened and
                      comment.issue_status in issue_status)

Hey,

Thanks for the change. Sorry this has totally fallen off my radar.

There's some things I'd like to see changed before this goes in.

Others get this wrong, but this should follow the proper docstring model:

"""One-line summary.

Multi-line description.
"""

Other columns get this wrong, but this should be:

super(StatusColumn, self).__init__...)

Same with IssueCountColumn.

This can be flattened by having this check == 0 and return ''. Fewer nesting if statements is good.

The strings are aligned weirdly.

Same comment about the docstring.

Same comment about alignment.

Each of these functions should have a docstring.

The "review_request" shouldn't be necessary. I think these would be best added to the ReviewRequest object. Then these would just operate on the ReviewRequest they're called on. Also resolves the inline import stuff.

Blank line after imports.

I think I'd prefer to just use return statements instead of setting a variable and returning in both cases. Shortens the code.

Blank line before this.

Blank line before this.

Blank line before this.

This sounds like it's the count of review requests. Maybe _get_comment_count.

This is really inefficient.

First off, these can be simplified with:

for review in review_request.reviews.filter(public=True)
for comment in review.get_all_comments(issue_status__in=issue_status)

That reduces what's called. Still, it causes a large number of queries. That can be further reduced with:


review_request.reviews.filter(
    Q(public=True) &
    (Q(comments__issue_status__in=issue_status)
    (Q(screenshot_comments__issue_status__in=issue_status)
    (Q(file_attachment_comments__issue_status__in=issue_status)))


Then you can avoid sum(), and just use .count().

Same comments apply to all these.

Indentation would be best as:

initializer=lambda r:
    (r.pk and r.get_open_issue_count() or 0)

Blank line before this.

Weird indentation.

Should use parens for multi-line conditionals.

Also, blank line before the first if statement here.

This is all getting quite repetitive. How about:

comment_fields = ['comments', 'screenshot_comments', 'file_attachment_comments']

for comment_field in comment_fields:
    comments = getattr(self, comment_field)

    for comment in comments:
        ...

Each type of operation should be tested separately, so that we can know exactly what is failing when.

This is too long. Can you do:

{% if entry.review.ship_it %}
<div class="...">
 {% trans ... %}
 {% if issue_open_count %} ... {% endif %}
</div>
{% endif %}

Might as well add closed counts too.

Might as well do this for closed and total counts too.

what can be done to prevent updating reviewrequest last_updated during database evolution and init of new fields?

Indeed, all existing reviewrequests have their last_updated field modified during this migration :-(
https://code.google.com/p/reviewboard/issues/detail?id=2723#c19

Hey,
Just letting you know that I've put together a set of changes that accomplishes this same goal, with some small differences in design and implementation. Much of the code in Review Board has changed since this change, so there wasn't a ton to reuse, but it was heavily inspired by the change, so thanks!
You'll see this in RB 2.0 beta 3.