Add visual indicators to new page entries and new comments, in order to highlight active/new discussions.

Review Request #8673 — Created Jan. 28, 2017 and discarded

Information

Review Board
master

Reviewers

On review requests, it can be difficult to discern new discussion from the old. This particularly pertains to finding new reviews and new comments on review requests with a lot of discussion.

This set of changes adds visual indicators/highlighting to all page entries and all comments that were posted since the user last visited the review request.

It essentially does this by adding reviews and comments to a CSS class if they were posted since the user last visited the review request. It then applies the visual indicators to the page using only LessCSS.

  • Checked if last_visited is passed down to the appropriate templates correctly.
  • Checked if the new-page-entry class and the new-comment class are applied when the conditional is true.
  • Checked if the page is updated with the correct styling when page-entries and comments are newer than last_visited (ie. indicators are added).
  • Checked if the visual indicators are removed when page-entries and comments are older than last_visited (ie. page refreshed on a page with page-entries/comments with the indicators).
  • Checked if the visual indicators are added and removed correctly in spite of any caching that might happen (ie. caching does not interfere with addition/removal).
  • Ran unit tests for checking if new-page-entry and new-comment are both added and removed correctly.

Description From Last Updated

I think if both the review and replies are new, we can just leave the whole box shaded and avoid …

daviddavid

Col: 5 E265 block comment should start with '# '

reviewbotreviewbot

Col: 5 E265 block comment should start with '# '

reviewbotreviewbot

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

reviewbotreviewbot

Col: 114 E202 whitespace before ')'

reviewbotreviewbot

Col: 77 W291 trailing whitespace

reviewbotreviewbot

Col: 9 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 74 W291 trailing whitespace

reviewbotreviewbot

Undo this change.

brenniebrennie

No newline here.

brenniebrennie

CSS selectors should be ordered as: &.foo {} a {} .foo {} #foo{} Within each group the selectors should be …

brenniebrennie

I do not know that this comment. is necessary.

brenniebrennie

Alphabetize.

brenniebrennie

The draft rendering and the new-comment rendering should be mutually exclusive. Additionally, producing an element with two different class= attribtues …

brenniebrennie

This line looks like it got unintentionally indented 1 space.

daviddavid

It doesn't seem useful to have separate definitions for entry and comment which are the same. Can we merge these?

daviddavid

The #reviews .review-request-page-entry part can be consolidated, like: #reviews .review-request-page-entry { &.new-page-entry { ... } &:not(.new-page-entry) { ... } }

chipx86chipx86

The 4px should also be in a constant.

chipx86chipx86

What's this here for?

chipx86chipx86

Maybe when wrapping and using multiple keyword arguments, you can have one keyword argument per line since there are lots …

RK rkdhatt

'FileAttachment' imported but unused

reviewbotreviewbot

'Screenshot' imported but unused

reviewbotreviewbot

'ReviewRequest' imported but unused

reviewbotreviewbot

'ReviewRequestDraft' imported but unused

reviewbotreviewbot

For unit test descriptions (modern ones, anyway), we try to start off with the component/page that's being tested, followed by …

chipx86chipx86

self.client.login will return the user, so you can use that directly. Same below.

chipx86chipx86

These can be one statement. Same below.

chipx86chipx86

local variable 'review' is assigned to but never used

reviewbotreviewbot

Older tests don't do this, but we should actually use local_site_reverse to get the URL instead of hard-coding it. Also, …

chipx86chipx86

It would be better to check for something more specific here (and below), since "new-page-entry" could theoretically be anywhere.

chipx86chipx86

Can you wrap the comparison in parenthesis? Helps to visually distinguish those.

chipx86chipx86

Let's put the ( on the line with the format strings. That way, all entries have a nice 4 space …

chipx86chipx86

Can you add a comment about this? I know we talked about this a bit, but wouldn't we want this …

chipx86chipx86

There's actually a little template tag you can use here for the class building: <li{% attr "class" %}{% if draft …

chipx86chipx86

'FileAttachment' imported but unused

reviewbotreviewbot

'ReviewRequestDraft' imported but unused

reviewbotreviewbot

'ReviewRequest' imported but unused

reviewbotreviewbot

'Screenshot' imported but unused

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

'FileAttachment' imported but unused

reviewbotreviewbot

'ReviewRequestDraft' imported but unused

reviewbotreviewbot

'ReviewRequest' imported but unused

reviewbotreviewbot

'Screenshot' imported but unused

reviewbotreviewbot

I would use docstring comment here (triple quotes)

MO Mons

maybe you should remove this blank line (unless that's the style guideline used elsewhere)

MO Mons

I like the fact that you put these on new lines. Makes it more readable in my opinion.

MO Mons

I would add a new blank line here. Seems like that's the general style in this file (blank line after …

MO Mons

is it possible to split this div into two lines?

MO Mons

same here.

MO Mons

is it possible to split this li tag into two lines?

MO Mons

'FileAttachment' imported but unused

reviewbotreviewbot

'Screenshot' imported but unused

reviewbotreviewbot

'ReviewRequest' imported but unused

reviewbotreviewbot

'ReviewRequestDraft' imported but unused

reviewbotreviewbot

We prefer to format formatting statements like this as : text = ( ' .... ' % foo )

brenniebrennie

Same here.

brenniebrennie

And here.

brenniebrennie

Here too.

brenniebrennie

ETag

brenniebrennie

'FileAttachment' imported but unused

reviewbotreviewbot

'ReviewRequestDraft' imported but unused

reviewbotreviewbot

'ReviewRequest' imported but unused

reviewbotreviewbot

'Screenshot' imported but unused

reviewbotreviewbot

'FileAttachment' imported but unused

reviewbotreviewbot

'ReviewRequestDraft' imported but unused

reviewbotreviewbot

'ReviewRequest' imported but unused

reviewbotreviewbot

'Screenshot' imported but unused

reviewbotreviewbot

'FileAttachment' imported but unused

reviewbotreviewbot

'search' imported but unused

reviewbotreviewbot

Col: 17 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 17 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 17 E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Col: 17 E126 continuation line over-indented for hanging indent

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/templates/reviews/review_reply.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/templates/reviews/review_reply.html
    
    
  2. Col: 5
     E265 block comment should start with '# '
    
  3. Col: 5
     E265 block comment should start with '# '
    
  4. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (116 > 79 characters)
    
  5. reviewboard/reviews/views.py (Diff revision 1)
     
     
    Col: 114
     E202 whitespace before ')'
    
  6. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/templates/reviews/review_reply.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/templates/reviews/review_reply.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 77
     W291 trailing whitespace
    
  3. reviewboard/reviews/views.py (Diff revision 2)
     
     
    Col: 9
     E128 continuation line under-indented for visual indent
    
  4. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/templates/reviews/review_reply.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/templates/reviews/review_reply.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 3)
     
     
    Col: 74
     W291 trailing whitespace
    
  3. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. 
      
brennie
  1. 
      
  2. Undo this change.

  3. No newline here.

  4. reviewboard/static/rb/css/pages/reviews.less (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    CSS selectors should be ordered as:

    &.foo {}
    a {}
    .foo {}
    #foo{}
    

    Within each group the selectors should be ordered alphabetically.

  5. I do not know that this comment. is necessary.

  6. Alphabetize.

  7. The draft rendering and the new-comment rendering should be mutually exclusive. Additionally, producing an element with two different class= attribtues is not valid. Move the class attribute generation into an if-elif.

  8. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. 
      
brennie
  1. Ship It!
  2. 
      
david
  1. 
      
  2. I think if both the review and replies are new, we can just leave the whole box shaded and avoid the extra border in the middle.

  3. This line looks like it got unintentionally indented 1 space.

  4. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/static/rb/css/pages/reviews.less (Diff revision 7)
     
     
     
     
     

    It doesn't seem useful to have separate definitions for entry and comment which are the same. Can we merge these?

    1. This was intentional, for the future in the case when someone wanted to set them each to different colours.

      But you're right, that doesn't seem very useful, especially when we don't need it right now.

  3. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/static/rb/css/pages/reviews.less (Diff revision 8)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    The #reviews .review-request-page-entry part can be consolidated, like:

    #reviews .review-request-page-entry {
        &.new-page-entry {
            ...
        }
    
        &:not(.new-page-entry) {
            ...
        }
    }
    
  3. The 4px should also be in a constant.

  4. What's this here for?

    1. I put this here because setting clear: both (which is done in #reviews .review .body) causes a gap between the header and footer of reviews. This isn't normally visible, but adding a left border to both .header and .body shows the gap.

      There are no visible differences I can see when I set this to clear: none, besides the gap disappearing. It might be legacy thing. However, I should have brought this up earlier, this isn't the best solution.

      Is there a better way to do this?

    2. Can you show me in Slack what it looks like with/without this, so I can get a better idea of its impact?

      Also, can you check in mobile mode? (Resize your browser to be smaller, width-wise, or go into the developer tools' Responsive Design Mode.) It may be that we have this for that purpose.

  5. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. 
      
RK
  1. 
      
  2. reviewboard/reviews/views.py (Diff revision 9)
     
     

    Maybe when wrapping and using multiple keyword arguments, you can have one keyword argument per line since there are lots of arguments (better readability).

    ex:
    etag = encode_tag(
    '%s:%s:%s:%s:%s:%s:%s:%s:%s:%s:%s' %
    request.user,
    last_activity_time,
    draft_timestamp,
    ...
    )

    1. I agree with that point - though I wouldn't necessarily put the '%s...' string on a next line, like in the example above.

    2. Holy smokes, ignore my last comment about putting the %s... string on the next line. I didn't realize it was already that way (and properly indented the 4 spaces).

      So please ignore my comment there. But I do support the idea of breaking up the individual elements being inserted into the string onto their own lines.

    3. Thanks for the feedback, guys!

      I did not format everything according to Raman's example, but I did put each argument on its own line.

      Mike, it seems that the '%s:%s... was NOT indented 4 spaces. It was indented 3 spaces for whatever reason. That's been corrected now.

  3. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 11)
     
     
     'FileAttachment' imported but unused
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 11)
     
     
     'Screenshot' imported but unused
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 11)
     
     
     'ReviewRequest' imported but unused
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 11)
     
     
     'ReviewRequestDraft' imported but unused
    
  6. reviewboard/reviews/tests/test_views.py (Diff revision 11)
     
     
     local variable 'review' is assigned to but never used
    
  7. 
      
chipx86
  1. This is looking great! Some recommendations for styling and just some conventions we have in our codebase, but this is close to landing!

  2. reviewboard/reviews/tests/test_views.py (Diff revision 11)
     
     
     

    For unit test descriptions (modern ones, anyway), we try to start off with the component/page that's being tested, followed by the condition. So, something more like: "Testing review_detail view sets new-page-entry CSS class on reviews when ..."

    ("CSS class" helps distinguish from Python classes.)

    There should also be one unit test per condition, so one for ensuring the class is set when it should be, one for ensuring the class is not set when it you've visited since. It's fine to repeat some of the setup logic that may have been covered in another unit test. What having multiple tests ultimately does is help when things go wrong. For instance, if the latter case fails, but the former doesn't, there's less diagnostics that have to happen to know what actually failed.

    Same applies below.

  3. reviewboard/reviews/tests/test_views.py (Diff revision 11)
     
     
     

    self.client.login will return the user, so you can use that directly.

    Same below.

    1. It is giving me errors when I set

      user = self.client.login(username='doc', password='doc')
      

      instead of what I had originally.

      self.client.login is returning a boolean whether or not the login was successful, rather than the user. That is what the Django docs say: https://docs.djangoproject.com/en/1.10/topics/testing/tools/#django.test.Client.login

      I also printed user to console and I get True.

  4. reviewboard/reviews/tests/test_views.py (Diff revision 11)
     
     
     
     

    These can be one statement.

    Same below.

  5. reviewboard/reviews/tests/test_views.py (Diff revision 11)
     
     

    Older tests don't do this, but we should actually use local_site_reverse to get the URL instead of hard-coding it.

    Also, when referencing IDs, you should use .pk instead of .id. With review requests, though, when you're dealing with URLs or for display purposes, you should always use .display_id.

    Same below.

  6. reviewboard/reviews/tests/test_views.py (Diff revision 11)
     
     

    It would be better to check for something more specific here (and below), since "new-page-entry" could theoretically be anywhere.

  7. reviewboard/reviews/views.py (Diff revision 11)
     
     

    Can you wrap the comparison in parenthesis? Helps to visually distinguish those.

  8. reviewboard/reviews/views.py (Diff revision 11)
     
     
     

    Let's put the ( on the line with the format strings. That way, all entries have a nice 4 space indentation.

  9. Can you add a comment about this?

    I know we talked about this a bit, but wouldn't we want this to actually be in the base CSS for this element, rather than right here? It seems like we'd need to be consistent. Either always have it or never.

    1. Can you clarify what you mean by "the base CSS for this element"? Do you mean #reviews .review .body or do you mean something else?

    2. Yeah, #reviews .review .body. Basically, from the screenshot you showed me, clear: none is needed to solve a visual defect. That means it's either not at all required and there's no reason to clear here (so clear: none is appropriate) or we must clear here to work around something and that thing we're working around will still be broken for new entries (because you're undoing the clear: both). It's either correct to clear in both places, or it's incorrect to clear in both places, but there must be consistency.

      I think we don't need the clear: both that we have in #reviews .review .body. That line has been there a long time, and I believe it's from when we used to show a little Ship It emblem on the top-left of the review. So instead of having clear: none here, just remove the clear: both.

    3. Thanks for the clarification. I agree. It's been removed now.

  10. There's actually a little template tag you can use here for the class building:

    <li{% attr "class" %}{% if draft %}draft{% elif last_visited <= timestamp %}new-comment{% endif %}{% endattr %} ...>
    

    That will conditionally add the "class" attribute if there's any content to put in it.

  11. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
     'FileAttachment' imported but unused
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
     'ReviewRequestDraft' imported but unused
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
     'ReviewRequest' imported but unused
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
     'Screenshot' imported but unused
    
  6. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  7. reviewboard/reviews/tests/test_views.py (Diff revision 12)
     
     
    Col: 80
     E501 line too long (80 > 79 characters)
    
  8. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
     'FileAttachment' imported but unused
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
     'ReviewRequestDraft' imported but unused
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
     'ReviewRequest' imported but unused
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     
     'Screenshot' imported but unused
    
  6. 
      
MO
  1. 
      
  2. reviewboard/reviews/tests/test_views.py (Diff revision 13)
     
     

    I would use docstring comment here (triple quotes)

    1. I thought docstrings were only for "public modules, functions, classes, and methods" (from https://www.python.org/dev/peps/pep-0008/#documentation-strings)?

      This is a block comment, so I think using only #'s works here.

  3. reviewboard/reviews/views.py (Diff revision 13)
     
     

    maybe you should remove this blank line (unless that's the style guideline used elsewhere)

  4. reviewboard/reviews/views.py (Diff revision 13)
     
     

    I like the fact that you put these on new lines. Makes it more readable in my opinion.

    1. Thank Raman! This was her suggestion! :)

  5. I would add a new blank line here. Seems like that's the general style in this file (blank line after a multiline comment).

  6. is it possible to split this div into two lines?

    1. I don't think we're concerned with any character limit for each line when it comes to HTML. There are a lot of other templates that have even longer elements on some lines.

  7. is it possible to split this li tag into two lines?

  8. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     
     'FileAttachment' imported but unused
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     
     'Screenshot' imported but unused
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     
     'ReviewRequest' imported but unused
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     
     'ReviewRequestDraft' imported but unused
    
  6. 
      
brennie
  1. 
      
  2. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     
     

    We prefer to format formatting statements like this as :

    text = (
        ' .... '
        % foo
    )
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     
     

    Same here.

  4. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     
     

    And here.

  5. reviewboard/reviews/tests/test_views.py (Diff revision 14)
     
     
     

    Here too.

  6. reviewboard/reviews/views.py (Diff revision 14)
     
     
  7. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 15)
     
     
     'FileAttachment' imported but unused
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 15)
     
     
     'ReviewRequestDraft' imported but unused
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 15)
     
     
     'ReviewRequest' imported but unused
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 15)
     
     
     'Screenshot' imported but unused
    
  6. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 16)
     
     
     'FileAttachment' imported but unused
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 16)
     
     
     'ReviewRequestDraft' imported but unused
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 16)
     
     
     'ReviewRequest' imported but unused
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 16)
     
     
     'Screenshot' imported but unused
    
  6. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 17)
     
     
     'FileAttachment' imported but unused
    
  3. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. reviewboard/reviews/tests/test_views.py (Diff revision 19)
     
     
     'search' imported but unused
    
  3. reviewboard/reviews/tests/test_views.py (Diff revision 19)
     
     
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  4. reviewboard/reviews/tests/test_views.py (Diff revision 19)
     
     
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  5. reviewboard/reviews/tests/test_views.py (Diff revision 19)
     
     
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  6. reviewboard/reviews/tests/test_views.py (Diff revision 19)
     
     
    Col: 17
     E126 continuation line over-indented for hanging indent
    
  7. 
      
szhang
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/tests/test_views.py
        reviewboard/reviews/templatetags/reviewtags.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
        reviewboard/templates/reviews/review_reply.html
        reviewboard/templates/reviews/boxes/change.html
    
    
  2. 
      
chipx86
Review request changed

Status: Discarded

Change Summary:

Superceded by https://reviews.reviewboard.org/r/9182/

Loading...