flake8
-
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 1) Show all issues -
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 1) E226 missing whitespace around arithmetic operator
Review Request #10715 — Created Sept. 15, 2019 and updated
When adding minified files for review the entire file is displayed in
the diff veiwer. Since min files are just one long line this does not
make sense to display as they are generally ignored in the review
process. With this change files matching the search .min. are hidden
by default with the option to display them if the reviewer clicks
"Show content". This is similar to how deleted files are handled in the
diff viewer. Since deleted and hidden files are very similar in
functionality I merged the fields into one hidden field with an
optional hidden_context if there is need to know the reason for a file
being hidden.
Included tests for these changes.
Summary | ID |
---|---|
5773f493e25a882435a9b80385d25c4cd3bfa25c | |
276cef5046ec8eca751fb4ddfe876f6a47320744 | |
4ff5f0819b4b6ffa1e61b3a5c66c3173d5d97caa | |
4c9aef2a23627d1bbf44bcd0cefa63fdd33c8d21 |
Description | From | Last Updated |
---|---|---|
Can you flesh out your description and testing done? See https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea#writinggoodchangedescriptions |
|
|
We should probably explain why it is hidden instead of just stating that it is. e.g., This file was hidden … |
|
|
E122 continuation line missing indentation or outdented |
![]() |
|
E226 missing whitespace around arithmetic operator |
![]() |
|
Single quotes for strings. |
|
|
It is more Pythonic to do elif '.min.' in depot_filename: (and also more readable). |
|
|
nit: blank line between these. |
|
|
This only tests the cumulative diff (ie the squashed version of all the commits) case. Can you add a a … |
|
|
nit: blank line between these. |
|
|
nit: we do django template indentation a little differently: the indentation occurs inside the block tags. Additionally, HTML is indented … |
|
|
Let's expand this comment just a little bit. How about: # Choose the most important information to show for why … |
|
|
I know it's not part of your change per se, but this is leftover debug output and can be removed. |
|
|
Can we move these new tests to be at the end of this suite? Right now they break up a … |
|
|
How about "Testing get_diff_files hiding of deleted/minified files for a diffset with history"? That line can wrap as needed. |
|
|
Please change out the " quotes for ' quotes. |
|
|
How about "Testing get_diff_files hiding of deleted/minified files for an individual commit"? That line can wrap as needed. |
|
|
Please change out the " quotes for ' quotes. |
|
|
This needs to use an exact equality comparison operator (===) |
|
|
The way this is laid out is a little bit error-prone (we're kind of interleaving the if/else clauses and HTML … |
|
|
This doesn't really make sense. Either a file is empty (no content) or minified (lots of content all squished together). … |
|
|
Please undo. This is here for debugging test failures. |
|
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 1) |
---|
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 1) |
---|
E226 missing whitespace around arithmetic operator
Description: |
|
---|
Commits: |
|
|||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+1956 -1806) |
We should probably explain why it is hidden instead of just stating that it is. e.g.,
This file was hidden because it is likely minified. Show content.
reviewboard/diffviewer/diffutils.py (Diff revision 2) |
---|
It is more Pythonic to do
elif '.min.' in depot_filename:
(and also more readable).
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 2) |
---|
This only tests the cumulative diff (ie the squashed version of all the commits) case.
Can you add a a test to make sure it works for the individual commits as well?
reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 2) |
---|
nit: we do django template indentation a little differently: the indentation occurs inside the block tags. Additionally, HTML is indented one space (irrespective of the django tag indentation), e.g.:
{% elif file.hide and not show_deleted %} {% if file.hide_context == 'deleted' %} <tbody> <tr> <td colspan="4"> {% if file.num_changes == 0 %} {% trans "..." %}
Commits: |
|
||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+298 -50) |
Commits: |
|
|||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+297 -51) |
Commits: |
|
|||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+296 -50) |
reviewboard/diffviewer/diffutils.py (Diff revision 5) |
---|
Let's expand this comment just a little bit. How about:
# Choose the most important information to show for why a # file is hidden. If a minified file is deleted, prefer to # show it as deleted.
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 5) |
---|
I know it's not part of your change per se, but this is leftover debug output and can be removed.
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 5) |
---|
Can we move these new tests to be at the end of this suite? Right now they break up a bunch of other more related tests.
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 5) |
---|
How about "Testing get_diff_files hiding of deleted/minified files for a diffset with history"?
That line can wrap as needed.
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 5) |
---|
Please change out the
"
quotes for'
quotes.
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 5) |
---|
How about "Testing get_diff_files hiding of deleted/minified files for an individual commit"?
That line can wrap as needed.
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 5) |
---|
Please change out the
"
quotes for'
quotes.
reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.es6.js (Diff revision 5) |
---|
This needs to use an exact equality comparison operator (
===
)
reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 5) |
---|
The way this is laid out is a little bit error-prone (we're kind of interleaving the if/else clauses and HTML tags). We can simplify it a lot by pulling the
tbody
class name out into a variable:{% definevar "tbody_class" %}{% spaceless %} {% if file.hide_context == "deleted" %} deleted {% else %} hidden {% endif %} {% endspaceless %}{% enddefinevar %} <tbody class="{{tbody_class}}"> <tr> <td colspan="4"> {% if file.hide_context == "deleted" %} ... {% endif %} </td> </tr> </tbody>
reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 5) |
---|
This doesn't really make sense. Either a file is empty (no content) or minified (lots of content all squished together).
I think if a file is empty we should always show the "This is an empty file" line.
Commits: |
|
|||||||||
---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+388 -136) |
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 6) |
---|
Please undo. This is here for debugging test failures.
Description: |
|
|||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Commits: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+393 -137) |