Add hiding min files by default

Review Request #10715 — Created Sept. 15, 2019 and updated

perplex
Review Board
master
reviewboard, students

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.



Summary
Address Barret's comments.
Address David's comments.
Loading file attachments...

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

daviddavid

We should probably explain why it is hidden instead of just stating that it is. e.g., This file was hidden ...

brenniebrennie

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E226 missing whitespace around arithmetic operator

reviewbotreviewbot

Single quotes for strings.

brenniebrennie

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

brenniebrennie

nit: blank line between these.

brenniebrennie

This only tests the cumulative diff (ie the squashed version of all the commits) case. Can you add a a ...

brenniebrennie

nit: blank line between these.

brenniebrennie

nit: we do django template indentation a little differently: the indentation occurs inside the block tags. Additionally, HTML is indented ...

brenniebrennie

Let's expand this comment just a little bit. How about: # Choose the most important information to show for why ...

daviddavid

I know it's not part of your change per se, but this is leftover debug output and can be removed.

daviddavid

Can we move these new tests to be at the end of this suite? Right now they break up a ...

daviddavid

How about "Testing get_diff_files hiding of deleted/minified files for a diffset with history"? That line can wrap as needed.

daviddavid

Please change out the " quotes for ' quotes.

daviddavid

How about "Testing get_diff_files hiding of deleted/minified files for an individual commit"? That line can wrap as needed.

daviddavid

Please change out the " quotes for ' quotes.

daviddavid

This needs to use an exact equality comparison operator (===)

daviddavid

The way this is laid out is a little bit error-prone (we're kind of interleaving the if/else clauses and HTML ...

daviddavid

This doesn't really make sense. Either a file is empty (no content) or minified (lots of content all squished together). ...

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

flake8

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

    1. 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.

  3. 
      
PE
PE
brennie
  1. 
      
  2. 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.
    
  3. reviewboard/diffviewer/diffutils.py (Diff revision 2)
     
     

    Single quotes for strings.

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

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

  5. nit: blank line between these.

  6. 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?

  7. nit: blank line between these.

  8. 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 "..." %}
    
  9. 
      
PE
PE
PE
david
  1. 
      
  2. 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.
    
  3. I know it's not part of your change per se, but this is leftover debug output and can be removed.

  4. 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.

  5. How about "Testing get_diff_files hiding of deleted/minified files for a diffset with history"?

    That line can wrap as needed.

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

    Please change out the " quotes for ' quotes.

  7. How about "Testing get_diff_files hiding of deleted/minified files for an individual commit"?

    That line can wrap as needed.

  8. Please change out the " quotes for ' quotes.

  9. This needs to use an exact equality comparison operator (===)

  10. 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>
    
  11. 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.

  12. 
      
PE
Review request changed
Loading...