Detect and handle collapsible minified file diffs

Review Request #10162 — Created Sept. 22, 2018 and updated

cdkushni
Review Board
release-4.0.x
60e9920...
reviewboard, students

Implementation of diff file name extension checking for '.min' and content
checking for a small number of long lines in a file. If either is true then
it is likely a minified file. The minified file is then collapsed similarly
to deleted files and can be expanded to show the content of the file.

Checks in diffutils.py before files chunks are created to check the file
name and the file contents on if it is a minified file.
In diffviewer/diffutils.py: If the file was previously marked as a min by
it's extension and if it has less than 20 lines total then the lines are
length checked to be greater than 500 characters and if true remarked as
minified files.

The diff_file_fragment.html then replaces then diff content with a minified
file message and a show content button that loads in the minified file.
Show content button handled with a event handler like the deleted content
event handler that loads in the diff with a spinner loader.



  • 0
  • 0
  • 27
  • 27
  • 54
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

mike_conley
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 1)
     
     

    One thing you might find useful is to use PDB (the Python Debugger), in tandem with print statements like this.

    You can make PDB break in your program by putting in:

    import pdb; pdb.set_trace()
    

    where you want the break to occur. Then, execute the steps to hit that line of code, and switch to your server window - you'll notice that the server has become a debugger. You can then next (n) through, continue (c) and print (p) things.

    1. I see, I'll try that next then, thanks!

  3. 
      
cdkushni
Review request changed

Commit:

-2df11efd2b0f40d52c337c4a6f0c855ecc0bb8d6
+22b8e752f07904bc9fd6cc3c2dd82818b388c045

Diff:

Revision 2 (+43)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

cdkushni
Review request changed

Summary:

-(WIP)Set up base file extension parsing to check for '.min.' extension before sending a bool with the result to the get_diff_files() data return. Still need to do the file line length checking.
+(WIP) checks if files are minified and adds the result to the file metadata.

Description:

~  

(WIP)Set up base file extension parsing to check for '.min.' extension before sending a bool with the result to the get_diff_files() data return. Still need to do the file line length checking.

  ~

(WIP)Set up base file extension parsing to check for '.min.' extension before

  + sending a bool with the result to the get_diff_files() data return.
  + Still need to do the file line length checking.

Commit:

-22b8e752f07904bc9fd6cc3c2dd82818b388c045
+6a45a10294cb2660791e935b55045497a42de8ef

Diff:

Revision 3 (+58)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

cdkushni
brennie
  1. 
      
  2. Unfortunately, Review Board never really has the files on the OS disk to read. The depot_filename that you've called this function with is the name of the file in the repository. To get the contents of this file, you would need to use get_patched_file.

    However, we do not want to do that just to see if the file is modified as this function is called by endpoints just to retrieve metadata about files in a diff.

    The JavaScript reposonsible for loading file contents for display in the DiffViewer is RB.DiffViewerPageView (in reviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js). Specifically in queueLoadDiff, which calls RB.DiffReviewableModel.getRenderedDiff (in reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.es6.js).

    The Django view for actually rendering the content (that the above JS model calls into) is reviewboard.diffviewer.views.DiffFragmentView (in reviewboard/diffviewer/views.py). This is where we will want to do the check call this function with (1) the filename and (2) the old/new file contents. Then we can take into account similar logic to how the contents of deleted files are requested to show the contents of minified files.

    I realize this is a lot to take in, but I'm happy to answer any questions (esp tomorrow in person) about this. I've spent quite a lot of time in the DiffViewer's JS recently, so I'm fairly familiar with how it all sort of fits together.

  3. reviewboard/diffviewer/diffutils.py (Diff revision 3)
     
     

    I find it easy to do import pdb; pdb.set_trace() wherever it is im using pdb. That way I don't forget to remove imports :)

  4. 
      
cdkushni
Review request changed

Commit:

-6a45a10294cb2660791e935b55045497a42de8ef
+97fd115f4b9339687d4f2a9f2864a47a1ca11ec7

Diff:

Revision 4 (+39)

Show changes

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

cdkushni
Review request changed

Summary:

-(WIP) checks if files are minified and adds the result to the file metadata.
+(WIP) checks if files are minified and overwrites the diff fragment display

Description:

   

Set up a function get_file_isminified(filename) in diffutils.py

    to check the file name and the file contents on if it is a minified file.
    This function is called and returns a boolean which is passed to the file
~   metadata in get_diff_files().

  ~ metadata in get_diff_files().
  + In diffviewer/renderer.py: If the file was previously marked as a min by
  + it's extension and if it has less than 20 lines total then the lines are
  + length checked to be greater than 800 characters and if true remarked as
  + minified files.
  + The diff_file_fragment.html then replaces then diff content with a minified
  + file message instead of displaying the contents.

Commit:

-97fd115f4b9339687d4f2a9f2864a47a1ca11ec7
+58a91172940a508410da813f9acc555268051520

Diff:

Revision 5 (+60 -5)

Show changes

Checks run (2 failed)

flake8 failed.
JSHint failed.

flake8

JSHint

cdkushni
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 passed.
JSHint failed.

JSHint

cdkushni
brennie
  1. 
      
  2. reviewboard/diffviewer/renderers.py (Diff revision 7)
     
     

    Nit: undo all these removed blank lines.

  3. reviewboard/diffviewer/renderers.py (Diff revision 7)
     
     
     
     
     
     
     

    We don't do multi-line comments with docstrings. Instead we do multiple single-line comments with #.

  4. reviewboard/diffviewer/renderers.py (Diff revision 7)
     
     
     

    Nit: blank line between statement and block.

  5. This will need to be a new class so we can use a separate event handler.

  6. 
      
cdkushni
Review request changed

Commit:

-80c23f9baef6c3273d78fab01bdf66aa2e39552a
+d792e26553ae3f9bb0ab780d2ac8c5926dc14212

Diff:

Revision 8 (+21 -6)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

cdkushni
cdkushni
cdkushni
cdkushni
Review request changed

Description:

   

Implementation of diff file name extension checking for '.min' and content

    checking for a small number of long lines in a file. If either is true then
    it is likely a minified file. The minified file is then collapsed similarly
    to deleted files and can be expanded to show the content of the file.

   
~  

Set up a check in diffutils.py to check the file name and the file contents

~   on if it is a minified file.
  ~

Checks in diffutils.py before files chunks are created to check the file

  ~ name and the file contents on if it is a minified file.
    In diffviewer/diffutils.py: If the file was previously marked as a min by
    it's extension and if it has less than 20 lines total then the lines are
~   length checked to be greater than 800 characters and if true remarked as
  ~ length checked to be greater than 500 characters and if true remarked as
    minified files.

   
   

The diff_file_fragment.html then replaces then diff content with a minified

    file message and a show content button that loads in the minified file.
    Show content button handled with a event handler like the deleted content
    event handler that loads in the diff with a spinner loader.

Loading...