Detect and handle collapsible minified file diffs
Review Request #10162 — Created Sept. 22, 2018 and updated
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.In
diffviewer/diffutils.py
,get_diff_files()
performs the file extension
check then inpopulate_diff_chunks()
if the file didn't have a.min
extension the content check is performed.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 with a event listener in
diffReviewableModel.es6.js
anddiffViewerPageView.es6.js
loads in the
minified file contents to the fragment.
test_diffutils.py
python test written for testing minification testing.
test_minified_file_detection
creates a minified diff object and verifies
thediffutils.py
populate_diff_chunks()
minification content check.Ran Python Unit Tests.
Ran Javascript Jasmine Unit Tests.
Description | From | Last Updated |
---|---|---|
Unfortunately, Review Board never really has the files on the OS disk to read. The depot_filename that you've called this … |
brennie | |
Please add a screenshot as a file attachment. |
david | |
Can you detail what testing you've done? |
david | |
W293 blank line contains whitespace |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E501 line too long (84 > 79 characters) |
reviewbot | |
W291 trailing whitespace |
reviewbot | |
E501 line too long (84 > 79 characters) |
reviewbot | |
F841 local variable 'temp' is assigned to but never used |
reviewbot | |
F841 local variable 'temp2' is assigned to but never used |
reviewbot | |
E501 line too long (92 > 79 characters) |
reviewbot | |
E261 at least two spaces before inline comment |
reviewbot | |
E265 block comment should start with '# ' |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
E265 block comment should start with '# ' |
reviewbot | |
E265 block comment should start with '# ' |
reviewbot | |
E265 block comment should start with '# ' |
reviewbot | |
E261 at least two spaces before inline comment |
reviewbot | |
F401 'pdb' imported but unused |
reviewbot | |
E302 expected 2 blank lines, found 1 |
reviewbot | |
E303 too many blank lines (2) |
reviewbot | |
Col: 1 Use the function form of "use strict". |
reviewbot | |
Col: 87 Expected an assignment or function call and instead saw an expression. |
reviewbot | |
Col: 153 Missing semicolon. |
reviewbot | |
Col: 154 Expected an assignment or function call and instead saw an expression. |
reviewbot | |
Col: 67 'module' is not defined. |
reviewbot | |
E303 too many blank lines (2) |
reviewbot | |
E501 line too long (83 > 79 characters) |
reviewbot | |
Col: 3 Expected an assignment or function call and instead saw an expression. |
reviewbot | |
Col: 9 Unclosed string. |
reviewbot | |
Col: 481 Unclosed string. |
reviewbot | |
Col: 2 Missing semicolon. |
reviewbot | |
Col: 1 Expected an assignment or function call and instead saw an expression. |
reviewbot | |
Col: 1 Unclosed string. |
reviewbot | |
Col: 67 'module' is not defined. |
reviewbot | |
Col: 154 Expected an assignment or function call and instead saw an expression. |
reviewbot | |
Col: 153 Missing semicolon. |
reviewbot | |
Col: 87 Expected an assignment or function call and instead saw an expression. |
reviewbot | |
Col: 1 Use the function form of "use strict". |
reviewbot | |
Col: 1 Expected an assignment or function call and instead saw an expression. |
reviewbot | |
Col: 2 Missing semicolon. |
reviewbot | |
Col: 481 Unclosed string. |
reviewbot | |
Col: 9 Unclosed string. |
reviewbot | |
Col: 3 Expected an assignment or function call and instead saw an expression. |
reviewbot | |
Col: 1 Unclosed string. |
reviewbot | |
Col: 1 Use the function form of "use strict". |
reviewbot | |
Col: 87 Expected an assignment or function call and instead saw an expression. |
reviewbot | |
Col: 153 Missing semicolon. |
reviewbot | |
Col: 154 Expected an assignment or function call and instead saw an expression. |
reviewbot | |
Col: 67 'module' is not defined. |
reviewbot | |
Nit: undo all these removed blank lines. |
brennie | |
We don't do multi-line comments with docstrings. Instead we do multiple single-line comments with #. |
brennie | |
Nit: blank line between statement and block. |
brennie | |
Undo. |
brennie | |
This will need to be a new class so we can use a separate event handler. |
brennie | |
E702 multiple statements on one line (semicolon) |
reviewbot | |
"minified_bool" isn't a good variable name (I can infer the type from the assignment), plus we only use it once. … |
david | |
Can we add a comment explaining what the heuristics are? |
david | |
This blank line isn't necessary. |
david | |
Can you add a doc comment for this? |
david | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
W191 indentation contains tabs |
reviewbot | |
E101 indentation contains mixed spaces and tabs |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E101 indentation contains mixed spaces and tabs |
reviewbot | |
E501 line too long (87 > 79 characters) |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E501 line too long (89 > 79 characters) |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E501 line too long (89 > 79 characters) |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E501 line too long (87 > 79 characters) |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E501 line too long (89 > 79 characters) |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E501 line too long (89 > 79 characters) |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E128 continuation line under-indented for visual indent |
reviewbot | |
E124 closing bracket does not match visual indentation |
reviewbot | |
W293 blank line contains whitespace |
reviewbot | |
Could use some rewording for clarity, like "Checks the number of lines in the chunk to determine if they are … |
shoven | |
Should there be an additional linebreak here to separate the two paragraphs? |
gojeffcho | |
This comment is a bit unclear, particularly the second part of it. I think the commenting might end up being … |
gojeffcho | |
I believe this should have an extra line break since the docstring is on a single line. |
gojeffcho | |
Tiny nit: two spaces in front of trans should probably be one. Also this line should be indented to be … |
shoven | |
W291 trailing whitespace |
reviewbot | |
Nitpick: I had to re-read this comment a few times to try and make sense of what it was saying. … |
skaefer143 |
-
-
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.
- Commit:
-
2df11efd2b0f40d52c337c4a6f0c855ecc0bb8d622b8e752f07904bc9fd6cc3c2dd82818b388c045
Checks run (1 failed, 1 succeeded)
flake8
- 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:
-
22b8e752f07904bc9fd6cc3c2dd82818b388c0456a45a10294cb2660791e935b55045497a42de8ef
- 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. ~ 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().
-
-
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 useget_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
(inreviewboard/static/rb/js/pages/views/diffViewerPageView.es6.js
). Specifically inqueueLoadDiff
, which callsRB.DiffReviewableModel.getRenderedDiff
(inreviewboard/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
(inreviewboard/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.
-
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 :)
- Commit:
-
6a45a10294cb2660791e935b55045497a42de8ef97fd115f4b9339687d4f2a9f2864a47a1ca11ec7
Checks run (2 failed)
flake8
JSHint
- 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:
-
97fd115f4b9339687d4f2a9f2864a47a1ca11ec758a91172940a508410da813f9acc555268051520
Checks run (2 failed)
flake8
JSHint
- Commit:
-
58a91172940a508410da813f9acc55526805152064bd4a34488669a224077567ff22f7f2c9151a97
Checks run (1 failed, 1 succeeded)
JSHint
- 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(). 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. ~ file message and (WIP) a show content button that loads in the minified file. - Commit:
-
64bd4a34488669a224077567ff22f7f2c9151a9780c23f9baef6c3273d78fab01bdf66aa2e39552a
Checks run (2 succeeded)
- 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(). ~ In diffviewer/renderer.py: If the file was previously marked as a min by ~ 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 minified files. The diff_file_fragment.html then replaces then diff content with a minified file message and (WIP) a show content button that loads in the minified file.
- Change Summary:
-
Completed the functionality of the collapsible minified diffs.
- 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. ~ Set up a check 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(). 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 minified files. The diff_file_fragment.html then replaces then diff content with a minified ~ file message and (WIP) a show content button that loads in the minified file. ~ 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. - Commit:
-
d792e26553ae3f9bb0ab780d2ac8c5926dc1421260e9920a2671b47d7b98e91132fd2676c873af7f
- Diff:
-
Revision 9 (+76 -8)
Checks run (2 succeeded)
- Summary:
-
(WIP) checks if files are minified and overwrites the diff fragment displayDetect and handle collapsible minified file diffs
- 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. 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 ~ minified files. ~ The diff_file_fragment.html then replaces then diff content with a minified ~ 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.
- 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.
- Testing Done:
-
+ DiffUtils python test written for testing minification testing.
- Commit:
-
60e9920a2671b47d7b98e91132fd2676c873af7f189fc75a38e3a1e14aa30bdf1ac7ee9510eb009c
- Diff:
-
Revision 10 (+146 -8)
Checks run (1 failed, 1 succeeded)
flake8
-
Warning: Showing 30 of 53 failures.
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
- Testing Done:
-
DiffUtils python test written for testing minification testing.
+ + Ran Python Unit Tests.
+ + Ran Javascript Jasmine Unit Tests.
- Commit:
-
189fc75a38e3a1e14aa30bdf1ac7ee9510eb009c1fd06cc6dfd8a2aa9216e5deba6cb8e7e40291c0
- Diff:
-
Revision 11 (+146 -8)
- Added Files:
- Commit:
-
1fd06cc6dfd8a2aa9216e5deba6cb8e7e40291c0eef9f3fa6dd42f29369e7d83ca4430274219fa45
- Diff:
-
Revision 12 (+146 -8)
Checks run (2 succeeded)
- Testing Done:
-
~ DiffUtils python test written for testing minification testing.
~ test_diffutils.py
python test written for testing minification testing.+ test_minified_file_detection
creates a minified diff object and tests+ the diffutils.py
populate_diff_chunks()
minification content check.Ran Python Unit Tests.
Ran Javascript Jasmine Unit Tests.
- Description:
-
~ Implementation of diff file name extension checking for '.min' and content
~ Implementation of diff file name extension checking for
.min
and contentchecking 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 ~ In
diffviewer/diffutils.py
,get_diff_files()
performs the file extension~ check then in populate_diff_chunks()
if the file didn't have a.min
~ extension the content check is performed. - 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. ~ 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 with a event listener in + diffReviewableModel.es6.js
anddiffViewerPageView.es6.js
loads in the+ minified file contents to the fragment. - Testing Done:
-
test_diffutils.py
python test written for testing minification testing.~ test_minified_file_detection
creates a minified diff object and tests~ test_minified_file_detection
creates a minified diff object and verifiesthe diffutils.py
populate_diff_chunks()
minification content check.Ran Python Unit Tests.
Ran Javascript Jasmine Unit Tests.
-
-
Could use some rewording for clarity, like "Checks the number of lines in the chunk to determine if they are short and not already categorized as minified files." (Based on your code it seems that you mean the number of lines in the chunk, not the length of the lines in the chunk so clarifying this in your comment would be helpful.)
Could be more clear on what "short" means (give a number)? It seems that you are saying a file is minified if the number of lines is less than 20?
-
Tiny nit: two spaces in front of
trans
should probably be one.Also this line should be indented to be nested within the
<td>
element.
- Commit:
-
eef9f3fa6dd42f29369e7d83ca4430274219fa45a8a1e36360fd8633c69717ef4ea2bcbe45bdf6a4
- Diff:
-
Revision 13 (+147 -8)
- Commit:
-
a8a1e36360fd8633c69717ef4ea2bcbe45bdf6a40dcef666ccf7f8ea7528d2acc986e6f3969a9a96
- Diff:
-
Revision 14 (+147 -8)
Checks run (2 succeeded)
-
-
Nitpick: I had to re-read this comment a few times to try and make sense of what it was saying. Could you try to make it a bit more clear? Maybe something like:
"It checks the file's chunk data to determine if it is possibly a minified file or not. If there are less than 20 lines, and more than 500 characters in a line, it is determined there is a minified script in the file."
Some thing like that? Correct my logic if it's wrong haha