Binary Diff Rendering

Review Request #10766 — Created Oct. 26, 2019 and updated

perplex
Review Board
master
reviewboard, students

Follows up on previous binary file changes. This will allow the frontend to render binary files for visual reviewing. This will only display a diff if the binary file is a image. If the file is a not a binary, exe for example, it will display this message 'This is a binary file. The content cannot be displayed.' This also fixes problems where if the same commit with a binary is upload multiple times the backend would hard fail. Now it returns without doing anything.



Summary
Fix frontend diff ui script not running and remove debug statements
use html instead of innerhtml
shorten line to adhere to max line length
Debug statements and some backend fixes
Description From Last Updated

Since there isn't any unit tests for these changes, it would be helpful if listed the testing that you did ...

amalik2amalik2

The title of the review request should also be more descriptive (it should be a complete sentence). I think there ...

amalik2amalik2

E501 line too long (97 > 79 characters)

reviewbotreviewbot

E501 line too long (102 > 79 characters)

reviewbotreviewbot

E722 do not use bare except'

reviewbotreviewbot

E501 line too long (103 > 79 characters)

reviewbotreviewbot

E501 line too long (103 > 79 characters)

reviewbotreviewbot

Col: 55 Expected ')' to match '(' from line 381 and instead saw 's'.

reviewbotreviewbot

Col: 56 Expected '{' and instead saw '.'.

reviewbotreviewbot

Col: 56 Expected an identifier and instead saw '.'.

reviewbotreviewbot

Col: 56 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 57 Missing semicolon.

reviewbotreviewbot

Col: 79 Missing semicolon.

reviewbotreviewbot

Col: 79 Expected an identifier and instead saw ')'.

reviewbotreviewbot

Col: 79 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 80 Missing semicolon.

reviewbotreviewbot

Col: 27 Expected an identifier and instead saw 'else'.

reviewbotreviewbot

Col: 27 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 31 Missing semicolon.

reviewbotreviewbot

E501 line too long (103 > 79 characters)

reviewbotreviewbot

Col: 55 Expected ')' to match '(' from line 381 and instead saw 's'.

reviewbotreviewbot

Col: 56 Expected '{' and instead saw '.'.

reviewbotreviewbot

Col: 56 Expected an identifier and instead saw '.'.

reviewbotreviewbot

Col: 56 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 57 Missing semicolon.

reviewbotreviewbot

Col: 79 Missing semicolon.

reviewbotreviewbot

Col: 79 Expected an identifier and instead saw ')'.

reviewbotreviewbot

Col: 79 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 80 Missing semicolon.

reviewbotreviewbot

Col: 27 Expected an identifier and instead saw 'else'.

reviewbotreviewbot

Col: 27 Expected an assignment or function call and instead saw an expression.

reviewbotreviewbot

Col: 31 Missing semicolon.

reviewbotreviewbot

Since orig_review_ui is a local variable that does not actually get used by the returned renderer, it may make more ...

amalik2amalik2

Same thing as the above comment

amalik2amalik2
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

PE
PE
Review request changed

Summary:

-WIP: Binary Diff Rendering
+Binary Diff Rendering

Description:

~  

Debug statements and some backend fixes

  ~

Follows up on previous binary file changes. This will allow the frontend to render binary files for visual reviewing. This will only display a diff if the binary file is a image. If the file is a not a binary, exe for example, it will display this message 'This is a binary file. The content cannot be displayed.' This also fixes problems where if the same commit with a binary is upload multiple times the backend would hard fail. Now it returns without doing anything.

Commits:

Summary
-
Debug statements and some backend fixes
+
Rebase onto FileDiff binary flag branch

Diff:

Revision 2 (+16 -22)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

PE
PE
Review request changed

Commits:

Summary
-
Rebase onto FileDiff binary flag branch
-
Fix line length for comment
+
Fix frontend diff ui script not running and remove debug statements
+
Debug statements and some backend fixes

Diff:

Revision 4 (+5911 -5899)

Show changes

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

PE
Review request changed

Commits:

Summary
-
Fix frontend diff ui script not running and remove debug statements
-
Debug statements and some backend fixes
+
Fix frontend diff ui script not running and remove debug statements
+
shorten line to adhere to max line length
+
Debug statements and some backend fixes

Diff:

Revision 5 (+5916 -5902)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

PE
Review request changed

Commits:

Summary
-
Fix frontend diff ui script not running and remove debug statements
-
shorten line to adhere to max line length
-
Debug statements and some backend fixes
+
Fix frontend diff ui script not running and remove debug statements
+
use html instead of innerhtml
+
shorten line to adhere to max line length
+
Debug statements and some backend fixes

Diff:

Revision 6 (+11595 -11615)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
amalik2
  1. 
      
  2. Since there isn't any unit tests for these changes, it would be helpful if listed the testing that you did for this review request as well.

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

    Since orig_review_ui is a local variable that does not actually get used by the returned renderer, it may make more sense to move this if statement to the beginning of this block

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

    Same thing as the above comment

  5. 
      
amalik2
  1. 
      
  2. The title of the review request should also be more descriptive (it should be a complete sentence). I think there was a page on notion that discusses this but I can't remember what it is, but you can look at recent review requests that have landed for an example.

    Each line in the description should be no more than 72 characters as well.

  3. 
      
Loading...