Ability to update attachments

Review Request #5824 — Created May 24, 2014 and discarded

Information

Review Board
800

Reviewers

This review allows users to update attachments with new version. See https://reviewboard.hackpad.com/Reviewing-attachments-tbfh4oiwTHB for description or https://www.youtube.com/watch?v=anX_rQKWSp0 for a quick demo.

Review includes: * Change to a schema to track sequences of file attachments * Modified attachment creation code to correctly assign the new parameters * Modified display logic to only show the latest element of each attachment sequence * Modified attachment review pages to include the slider

This review is aimed at project branch (see testing section for the problems description)

Performed various scenarios of adding/updating/reviewing attachments.

Current problems: * There is no beautiful UI * We are reloading the page each time slider is changed - API should be extended to load the necessary data dynamically.

Description From Last Updated

As per last week's meeting, we need to stick to existing terminolgoy as best as possible, instead of inventing new …

chipx86chipx86

'Max' imported but unused

reviewbotreviewbot

Col: 72 E502 the backslash is redundant between brackets

reviewbotreviewbot

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

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 1 E302 expected 2 blank lines, found 1

reviewbotreviewbot

Col: 61 E211 whitespace before '['

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 574

reviewbotreviewbot

Col: 17 E122 continuation line missing indentation or outdented

reviewbotreviewbot

Col: 61 E211 whitespace before '['

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 574

reviewbotreviewbot

Col: 17 E122 continuation line missing indentation or outdented

reviewbotreviewbot

Col: 61 E211 whitespace before '['

reviewbotreviewbot

'FileAttachmentHistory' imported but unused

reviewbotreviewbot

list comprehension redefines 'file_attachment' from line 576

reviewbotreviewbot

Instead of using FileAttachment.objects.filter(attachment_history=attachment_history), you can follow the reverse field (note that this particular code needs the related_name change I …

daviddavid

Why not just use the FileAttachmentHistory's id field as the display position? It's already a unique integer which is auto-incremented …

daviddavid

Col: 17 E122 continuation line missing indentation or outdented

reviewbotreviewbot

We use trailing commas for lists and dicts in python.

daviddavid

Care to add related_name='file_attachments' here?

daviddavid

If you set default=0, you don't have to set it when creating attachments that don't use an existing FileAttachmentHistory

daviddavid

This can follow the backwards relationship too: return self.file_attachments.count() - 1 Why don't you consider the first uploaded file to …

daviddavid

I think it's worth creating a method on FileAttachmentHistory to return the latest revision.

daviddavid

Col: 61 E211 whitespace before '['

reviewbotreviewbot

Can you add a docstring?

daviddavid

These can be combined and made more efficient using sorted: return sorted(self.get_latest_file_attachments(), key=get_display_position)

daviddavid

'FileAttachmentHistory' imported but unused

reviewbotreviewbot

Care to just put this in the dict definition for context_data? It's not worth defining a variable unless we use …

daviddavid

list comprehension redefines 'file_attachment' from line 576

reviewbotreviewbot

Same here.

daviddavid

This can use the backwards relationship.

daviddavid

Should have a trailing comma.

daviddavid

I think it might be nice to pull out the upload file view into a separate change. You already have …

daviddavid

It looks like 'e' isn't used in here, which will make jshint unhappy. Can you just remove it from the …

daviddavid

These can be simplified: this.$el.html('<div id="revision_selector" />');

daviddavid

Can you make this use /* */, since it's a multi-line comment?

daviddavid

Alignment is funky here.

daviddavid

This also needs to take into account the local site name and SITE_ROOT.

daviddavid

Leftover debug output?

daviddavid

If you put the review request into a variable in the containing method, you could just use it here without …

daviddavid

We generally put the space at the end of the first line instead of the beginning of the second.

daviddavid

This can use the reverse relation.

daviddavid

Col: 56 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 58 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 63 E502 the backslash is redundant between brackets

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 59 E502 the backslash is redundant between brackets

reviewbotreviewbot

Col: 13 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

Col: 60 E502 the backslash is redundant between brackets

reviewbotreviewbot

Col: 17 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

Col: 56 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 58 E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

Col: 63 E502 the backslash is redundant between brackets

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 18 E261 at least two spaces before inline comment

reviewbotreviewbot

Col: 59 E502 the backslash is redundant between brackets

reviewbotreviewbot

Col: 13 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

Col: 60 E502 the backslash is redundant between brackets

reviewbotreviewbot

Col: 17 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

Can this fit on one line?

PE PeterTran

Col: 36 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

Col: 40 E131 continuation line unaligned for hanging indent

reviewbotreviewbot

Is this a tab instead of white space?

PE PeterTran

Can this be hyphen-separated for consistency with your classes?

PE PeterTran

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
      Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
  2. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
      Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
    
    
  2. 
      
VO
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
      Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/templates/reviews/review_request_dlgs.html
    
    
  2. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
      Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/templates/reviews/review_request_dlgs.html
    
    
  2. 
      
chipx86
  1. 
      
  2. reviewboard/attachments/models.py (Diff revision 2)
     
     
     
     

    As per last week's meeting, we need to stick to existing terminolgoy as best as possible, instead of inventing new terminology.

    I strongly believe the right model is:

    1. A attachment_revision field for the revision of the file attachment.
    2. Some way of grouping related attachments (a SHA1 or some other unique identifier, rather than an integer, would be needed, or some other table, but a SHA1 is probably fine -- you'll need to make sure this is indexed in the Meta class for FileAttachment.)
    3. A custom "through" table for the M2M field between ReviewRequest and FileAttachment that specifies the visible position the file attachment is shown at in the list of thumbnails (since right now, it's based on timestamp or ID or something, and we don't want things to shift around).
    1. For 2. here, I think we should use another model (maybe FileAttachmentHistory, to match the DiffSetHistory elsewhere). This way we get consistency for free because we're just using a foreign key.

    2. Agreed. This is probably the best way to go.

    3. I created FileAttachmentHistory for the purpose of using it for groupping. I can't put anything useful there though, so it serves that same purpose as if I had this variable inside the class.
      I will look into part 3 shortly. The behaviour I'm thinking is that once the file attachment was added, it position stays the same (even if it gets updated). If the file collection was deleted, we preserve the relative ordering of the other files. If I will need extra field in the M2M table to achieve this behaviour, I will add it.

  3. 
      
VO
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
      Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/templates/reviews/review_request_dlgs.html
    
    
  2. reviewboard/attachments/forms.py (Diff revision 3)
     
     
    Col: 72
     E502 the backslash is redundant between brackets
    
  3. reviewboard/attachments/forms.py (Diff revision 3)
     
     
    Col: 17
     E128 continuation line under-indented for visual indent
    
  4. reviewboard/attachments/models.py (Diff revision 3)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
  5. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
      Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/templates/reviews/review_request_dlgs.html
    
    
  2. reviewboard/attachments/forms.py (Diff revision 3)
     
     
     'Max' imported but unused
    
  3. 
      
VO
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/webapi/resources/base_file_attachment.py
      Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/templates/reviews/review_request_dlgs.html
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/attachments/models.py (Diff revision 4)
     
     
    Col: 1
     E302 expected 2 blank lines, found 1
    
    1. I will fix these in the next iteration.

  3. Col: 61
     E211 whitespace before '['
    
  4. 
      
reviewbot
  1. This is a review from Review Bot.
      Tool: Pyflakes
      Processed Files:
        reviewboard/reviews/views.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/webapi/resources/base_file_attachment.py
      Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/templates/reviews/review_request_dlgs.html
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/reviews/views.py (Diff revision 4)
     
     
     list comprehension redefines 'file_attachment' from line 574
    
  3. 
      
VO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/staticbundles.py
        reviewboard/reviews/models/base_review_request_details.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/templates/reviews/review_request_dlgs.html
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/reviews/review_request_box.html
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/staticbundles.py
        reviewboard/reviews/models/base_review_request_details.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/templates/reviews/review_request_dlgs.html
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/templates/reviews/review_request_box.html
    
    
  2. reviewboard/attachments/forms.py (Diff revision 5)
     
     
    Col: 17
     E122 continuation line missing indentation or outdented
    
  3. Col: 61
     E211 whitespace before '['
    
  4. reviewboard/reviews/views.py (Diff revision 5)
     
     
     list comprehension redefines 'file_attachment' from line 574
    
  5. 
      
VO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/reviews/urls.py
        reviewboard/attachments/forms.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/reviews/ui/base.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/templates/reviews/review_request_dlgs.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/reviews/urls.py
        reviewboard/attachments/forms.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/reviews/ui/base.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/templates/reviews/review_request_dlgs.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
        reviewboard/static/rb/js/pages/views/diffViewerPageView.js
    
    
  2. reviewboard/attachments/forms.py (Diff revision 6)
     
     
    Col: 17
     E122 continuation line missing indentation or outdented
    
  3. Col: 61
     E211 whitespace before '['
    
  4. reviewboard/reviews/views.py (Diff revision 6)
     
     
     'FileAttachmentHistory' imported but unused
    
  5. reviewboard/reviews/views.py (Diff revision 6)
     
     
     list comprehension redefines 'file_attachment' from line 576
    
  6. 
      
VO
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/reviews/urls.py
        reviewboard/attachments/forms.py
        reviewboard/staticbundles.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/reviews/ui/base.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/templates/reviews/review_request_dlgs.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/reviews/urls.py
        reviewboard/attachments/forms.py
        reviewboard/staticbundles.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/reviews/ui/base.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/diffviewer/views/commonRevisionSelectorView.js
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/diffviewer/views/diffRevisionSelectorView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/diffviewer/views/attachmentRevisionSelectorView.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/templates/reviews/review_request_dlgs.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
  2. reviewboard/attachments/forms.py (Diff revision 7)
     
     
    Col: 17
     E122 continuation line missing indentation or outdented
    
  3. Col: 61
     E211 whitespace before '['
    
  4. reviewboard/reviews/views.py (Diff revision 7)
     
     
     'FileAttachmentHistory' imported but unused
    
  5. reviewboard/reviews/views.py (Diff revision 7)
     
     
     list comprehension redefines 'file_attachment' from line 576
    
  6. 
      
david
  1. 
      
  2. reviewboard/attachments/forms.py (Diff revision 7)
     
     
     

    Instead of using FileAttachment.objects.filter(attachment_history=attachment_history), you can follow the reverse field (note that this particular code needs the related_name change I asked for elsewhere):

    attachment_revision = (
        attachment_history.file_attachments
            .aggregate(Max('attachment_revision'))
            .get('attachment_revision__max')
        + 1)
    
    1. Nice, I didn't know about the reverse fields before.

  3. reviewboard/attachments/forms.py (Diff revision 7)
     
     
     
     
     

    Why not just use the FileAttachmentHistory's id field as the display position? It's already a unique integer which is auto-incremented any time a FileAttachmentHistory is created.

    1. Yeah, that's what I wanted to do. Per discussion with ChipX86, however, it was decided to have a separate field for position, as someone might implement attachment reordering. In order to do so easily, we would use a separate field for that, and whoever is implementing this change will just modify the behaviour of this field.

    2. OK. How about rewriting this particular code as such:

      if FileAttachmentHistory.objects.exists():
          attachment_history.display_position = 1
      else:
          attachment_history.display_position = (
              FileAttachmentHistory.objects
                  .aggregate(Max('display_position'))
                  .get('display_position__max') + 1)
      

      The changes here are not using the ternary form (which we generally don't like), some reformatting, and using .exists() instead of .count() (which is slightly more efficient use of the database).

    3. Cool, will do.

  4. reviewboard/attachments/forms.py (Diff revision 7)
     
     

    We use trailing commas for lists and dicts in python.

  5. reviewboard/attachments/models.py (Diff revision 7)
     
     

    Care to add related_name='file_attachments' here?

  6. reviewboard/attachments/models.py (Diff revision 7)
     
     

    If you set default=0, you don't have to set it when creating attachments that don't use an existing FileAttachmentHistory

  7. reviewboard/attachments/models.py (Diff revision 7)
     
     
     

    This can follow the backwards relationship too:

    return self.file_attachments.count() - 1
    

    Why don't you consider the first uploaded file to be a revision? That doesn't make sense to me. It's not like with diffs where the 'orig' version is upstream.

    1. Well, I was thinking of mimicing behaviour for diffs (you have an original file, which gets upgraded a coupld of times). When displaying it, I am using the "orig" label, just like the diffs do, so I need total - 1 extra selection points. So I used the same definition of revisions as diffs did.
      If you think we should not do that, I can return the count here and then subtract one in the front end part. OK?

  8. I think it's worth creating a method on FileAttachmentHistory to return the latest revision.

  9. Can you add a docstring?

  10. These can be combined and made more efficient using sorted:

    return sorted(self.get_latest_file_attachments(),
                  key=get_display_position)
    
  11. reviewboard/reviews/views.py (Diff revision 7)
     
     
     

    Care to just put this in the dict definition for context_data? It's not worth defining a variable unless we use it more than once.

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

    Same here.

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

    This can use the backwards relationship.

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

    Should have a trailing comma.

  15. I think it might be nice to pull out the upload file view into a separate change. You already have the view itself in a separate review request. That way we could move over to the view and get that code pushed before all the rest of this is finished.

    1. Done:
      https://reviews.reviewboard.org/r/5911/ (and also https://reviews.reviewboard.org/r/6120/)

  16. It looks like 'e' isn't used in here, which will make jshint unhappy. Can you just remove it from the argument list?

  17. These can be simplified:

    this.$el.html('<div id="revision_selector" />');
    
  18. Can you make this use /* */, since it's a multi-line comment?

  19. Alignment is funky here.

  20. This also needs to take into account the local site name and SITE_ROOT.

  21. Leftover debug output?

  22. If you put the review request into a variable in the containing method, you could just use it here without fetching it again.

  23. We generally put the space at the end of the first line instead of the beginning of the second.

  24. This can use the reverse relation.

  25. 
      
VO
VO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/attachments/forms.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/reviews/ui/base.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/reviews/ui/generic.py
        reviewboard/reviews/ui/__init__.py
        reviewboard/attachments/forms.py
        reviewboard/staticbundles.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/reviews/ui/base.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/genericReviewableView.js
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
  2. reviewboard/attachments/models.py (Diff revision 8)
     
     
    Col: 56
     E251 unexpected spaces around keyword / parameter equals
    
  3. reviewboard/attachments/models.py (Diff revision 8)
     
     
    Col: 58
     E251 unexpected spaces around keyword / parameter equals
    
  4. reviewboard/attachments/models.py (Diff revision 8)
     
     
    Col: 63
     E502 the backslash is redundant between brackets
    
  5. reviewboard/attachments/models.py (Diff revision 8)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  6. Col: 13
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Col: 59
     E502 the backslash is redundant between brackets
    
  8. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Col: 13
     E131 continuation line unaligned for hanging indent
    
  9. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Col: 60
     E502 the backslash is redundant between brackets
    
  10. reviewboard/reviews/views.py (Diff revision 8)
     
     
    Col: 17
     E131 continuation line unaligned for hanging indent
    
  11. reviewboard/attachments/models.py (Diff revision 9)
     
     
    Col: 56
     E251 unexpected spaces around keyword / parameter equals
    
  12. reviewboard/attachments/models.py (Diff revision 9)
     
     
    Col: 58
     E251 unexpected spaces around keyword / parameter equals
    
  13. reviewboard/attachments/models.py (Diff revision 9)
     
     
    Col: 63
     E502 the backslash is redundant between brackets
    
  14. reviewboard/attachments/models.py (Diff revision 9)
     
     
    Col: 13
     E128 continuation line under-indented for visual indent
    
  15. Col: 13
     E128 continuation line under-indented for visual indent
    
  16. reviewboard/reviews/ui/generic.py (Diff revision 9)
     
     
    Col: 18
     E261 at least two spaces before inline comment
    
  17. reviewboard/reviews/views.py (Diff revision 9)
     
     
    Col: 59
     E502 the backslash is redundant between brackets
    
  18. reviewboard/reviews/views.py (Diff revision 9)
     
     
    Col: 13
     E131 continuation line unaligned for hanging indent
    
  19. reviewboard/reviews/views.py (Diff revision 9)
     
     
    Col: 60
     E502 the backslash is redundant between brackets
    
  20. reviewboard/reviews/views.py (Diff revision 9)
     
     
    Col: 17
     E131 continuation line unaligned for hanging indent
    
  21. 
      
VO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/reviews/ui/generic.py
        reviewboard/reviews/ui/__init__.py
        reviewboard/attachments/forms.py
        reviewboard/staticbundles.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/reviews/ui/base.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/genericReviewableView.js
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
  2. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Col: 36
     E131 continuation line unaligned for hanging indent
    
  3. reviewboard/reviews/views.py (Diff revision 10)
     
     
    Col: 40
     E131 continuation line unaligned for hanging indent
    
  4. 
      
PE
  1. 
      
  2. Can this fit on one line?

  3. Is this a tab instead of white space?

  4. Can this be hyphen-separated for consistency with your classes?

    1. There are a bunch of these (e.g. diff_revision_selector), but that's an existing code, so I decided not to modify it. Maybe I'll make a separate review that fixes style problems in files that I had to work with.

  5. 
      
VO
VO
VO
VO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/reviews/ui/generic.py
        reviewboard/reviews/ui/__init__.py
        reviewboard/attachments/forms.py
        reviewboard/staticbundles.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/reviews/ui/base.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
        reviewboard/templates/reviews/parts/file_attachment_thumbnail.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/genericReviewableView.js
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/pages/views/reviewablePageView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/reviews/ui/generic.py
        reviewboard/attachments/evolutions/__init__.py
        reviewboard/reviews/ui/__init__.py
        reviewboard/attachments/evolutions/file_attachment_revision.py
        reviewboard/attachments/forms.py
        reviewboard/staticbundles.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/reviews/ui/base.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/genericReviewableView.js
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
    
    
  2. Col: 80
     E501 line too long (86 > 79 characters)
    
  3. Col: 80
     E501 line too long (133 > 79 characters)
    
  4. 
      
VO
VO
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/reviews/ui/generic.py
        reviewboard/attachments/evolutions/__init__.py
        reviewboard/reviews/ui/__init__.py
        reviewboard/attachments/evolutions/file_attachment_revision.py
        reviewboard/attachments/forms.py
        reviewboard/staticbundles.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/reviews/ui/base.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/genericReviewableView.js
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
    
    
  2. Col: 13
     E128 continuation line under-indented for visual indent
    
  3. Col: 13
     E128 continuation line under-indented for visual indent
    
  4. 
      
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/views.py
        reviewboard/webapi/resources/base_file_attachment.py
        reviewboard/attachments/models.py
        reviewboard/reviews/ui/generic.py
        reviewboard/attachments/evolutions/__init__.py
        reviewboard/reviews/ui/__init__.py
        reviewboard/attachments/evolutions/file_attachment_revision.py
        reviewboard/attachments/forms.py
        reviewboard/staticbundles.py
        reviewboard/reviews/models/base_review_request_details.py
        reviewboard/reviews/ui/base.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/fileAttachmentThumbnailView.js
        reviewboard/static/rb/js/views/fileAttachmentReviewableView.js
        reviewboard/templates/reviews/ui/text.html
        reviewboard/templates/reviews/review_request_box.html
        reviewboard/static/rb/js/views/textBasedReviewableView.js
        reviewboard/static/rb/js/views/genericReviewableView.js
        reviewboard/static/rb/js/models/fileAttachmentReviewableModel.js
        reviewboard/static/rb/js/views/imageReviewableView.js
    
    
  2. Col: 13
     E128 continuation line under-indented for visual indent
    
  3. Col: 13
     E128 continuation line under-indented for visual indent
    
  4. 
      
VO
Review request changed

Status: Discarded

Loading...