- Download Diff

Summary:

Add hiding min files by default

Review Request #10715 — Created Sept. 15, 2019 and updated Nov. 30, 2019, 4:20 p.m.

Information
Owner:	perplex
Repository:	Review Board
Branch:	master
Bugs:
Depends On:
Reviewers
Groups:	reviewboard, students
People:

Description

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.

Testing Done

Included tests for these changes.

Commits

Summary
Address Barret's comments.
Address David's comments.
re-add debug statement
rebase master

Screenshots

Files

Issues

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	david	Sept. 29, 2019, 1:51 p.m.
We should probably explain why it is hidden instead of just stating that it is. e.g., This file was hidden …	brennie	Sept. 29, 2019, 1:50 p.m.
E122 continuation line missing indentation or outdented	reviewbot	Sept. 16, 2019, 8:59 a.m.
E226 missing whitespace around arithmetic operator	reviewbot	Sept. 16, 2019, 8:59 a.m.
Single quotes for strings.	brennie	Sept. 29, 2019, 1:50 p.m.
It is more Pythonic to do elif '.min.' in depot_filename: (and also more readable).	brennie	Sept. 29, 2019, 1:50 p.m.
nit: blank line between these.	brennie	Sept. 29, 2019, 1:50 p.m.
This only tests the cumulative diff (ie the squashed version of all the commits) case. Can you add a a …	brennie	Sept. 29, 2019, 1:50 p.m.
nit: blank line between these.	brennie	Sept. 29, 2019, 1:51 p.m.
nit: we do django template indentation a little differently: the indentation occurs inside the block tags. Additionally, HTML is indented …	brennie	Sept. 29, 2019, 1:51 p.m.
Let's expand this comment just a little bit. How about: # Choose the most important information to show for why …	david	Oct. 11, 2019, 5:52 p.m.
I know it's not part of your change per se, but this is leftover debug output and can be removed.	david	Oct. 11, 2019, 5:52 p.m.
Can we move these new tests to be at the end of this suite? Right now they break up a …	david	Oct. 11, 2019, 5:52 p.m.
How about "Testing get_diff_files hiding of deleted/minified files for a diffset with history"? That line can wrap as needed.	david	Oct. 11, 2019, 5:52 p.m.
Please change out the " quotes for ' quotes.	david	Oct. 11, 2019, 5:52 p.m.
How about "Testing get_diff_files hiding of deleted/minified files for an individual commit"? That line can wrap as needed.	david	Oct. 11, 2019, 5:52 p.m.
Please change out the " quotes for ' quotes.	david	Oct. 11, 2019, 5:52 p.m.
This needs to use an exact equality comparison operator (===)	david	Oct. 11, 2019, 5:52 p.m.
The way this is laid out is a little bit error-prone (we're kind of interleaving the if/else clauses and HTML …	david	Oct. 11, 2019, 5:52 p.m.
This doesn't really make sense. Either a file is empty (no content) or minified (lots of content all squished together). …	david	Oct. 11, 2019, 5:52 p.m.
Please undo. This is here for debugging test failures.	brennie	Nov. 30, 2019, 4:20 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 1)
Show all issues
```
E122 continuation line missing indentation or outdented
```
reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 1)
Show all issues
```
E226 missing whitespace around arithmetic operator
```

Show all issues

Can you flesh out your description and testing done? See https://www.notion.so/reviewboard/Writing-Good-Change-Descriptions-10529e7c207743fa8ca90153d4b21fea#writinggoodchangedescriptions

perplex Sept. 15, 2019, 1:48 p.m.

For testing are you looking for more frontend, backend, or both. As it stands I have the one test for the backend the verifies that deletion and hidding are set correctly and that precedence is maintained. I.e. a min file deleted should show the deleted context not just that is was hidden.

Description:

~		Add hiding min files by default
	~	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 hiddent 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.

Commits:

	Summary	Author
-	Add hiding min files by default	ceegan
+	Add hiding min files by default	ceegan
+	Add whitespace around arithmetic operator and indetation to continuing line	ceegan

Diff:

Revision 2 (+1956 -1806)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/views.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/models/diffFileModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffFileModelTests.es6.js
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Hidden Min File.PNG (Revision 1)

Show all issues

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)
Show all issues
```
Single quotes for strings.
```

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

Show all issues

It is more Pythonic to do elif '.min.' in depot_filename: (and also more readable).

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 2)
Show all issues
```
nit: blank line between these.
```

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 2)

Show all issues

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/diffviewer/tests/test_diffutils.py (Diff revision 2)
Show all issues
```
nit: blank line between these.
```

reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 2)

Show all issues

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:

	Summary	Author
-	Add hiding min files by default	ceegan
-	Add whitespace around arithmetic operator and indetation to continuing line	ceegan
+	test	Ceegan Hale
+	Address Barret's comments.	Ceegan Hale

Diff:

Revision 3 (+298 -50)

Show changes

	reviewboard/test.min.txt
	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/models/diffFileModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffFileModelTests.es6.js
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

	Summary
-	test
-	Address Barret's comments.
+	test
+	Address Barret's comments.
+	Remove test file

Diff:

Revision 4 (+297 -51)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/models/diffFileModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffFileModelTests.es6.js
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.es6.js
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commits:

	Summary
-	test
-	Address Barret's comments.
-	Remove test file
+	Address Barret's comments.

Diff:

Revision 5 (+296 -50)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/models/diffFileModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffFileModelTests.es6.js
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.es6.js
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

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

Show all issues

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)

Show all issues

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)

Show all issues

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)

Show all issues

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)
Show all issues
```
Please change out the " quotes for ' quotes.
```

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 5)

Show all issues

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)
Show all issues
```
Please change out the " quotes for ' quotes.
```
reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.es6.js (Diff revision 5)
Show all issues
```
This needs to use an exact equality comparison operator (===)
```

reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 5)

Show all issues

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)

Show all issues

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:

	Summary
-	Address Barret's comments.
+	Address Barret's comments.
+	Address David's comments.

Diff:

Revision 6 (+388 -136)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/models/diffFileModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffFileModelTests.es6.js
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.es6.js
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 6)
Show all issues
```
Please undo. This is here for debugging test failures.
```

Description:

~		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 hiddent 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.
	~	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.

Testing Done:

Included tests for these changes.

Commits:

	Summary
-	Address Barret's comments.
-	Address David's comments.
+	Address Barret's comments.
+	Address David's comments.
+	re-add debug statement
+	rebase master

Diff:

Revision 7 (+393 -137)

Show changes

	reviewboard/diffviewer/diffutils.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/reviews/views.py
	reviewboard/static/rb/css/defs.less
	reviewboard/static/rb/css/pages/diffviewer.less
	reviewboard/static/rb/js/diffviewer/models/diffFileModel.es6.js
	reviewboard/static/rb/js/diffviewer/models/tests/diffFileModelTests.es6.js
	reviewboard/static/rb/js/diffviewer/views/diffFileIndexView.es6.js
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

You have a pending review.

Review Board 5.0.2 alpha 0 (dev)

Add hiding min files by default

flake8

Description:

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Description:

Testing Done:

Commits:

Diff:

Checks run (2 succeeded)