Model and UI changes to enable attachments for binary diffs

Review Request #2651 — Created Oct. 9, 2011 and discarded

Information

Review Board

Reviewers

Changed FileAttachment model to enable per-diff attachments; Enabled simple UI for displaying image attachments;
Manually adding/changing db entries to create binary attachments and verifying displaying images from the diff viewer page.
Description From Last Updated

Trailing whitespace.

chipx86chipx86

Do we know that this field is necessary?

chipx86chipx86

Two blank lines between sections. The imports must be in alphabetical order.

chipx86chipx86

Trailing whitespace. Can you go through the rest of the file and fix those?

chipx86chipx86

Make sure all the HTML uses 1 space indenting.

chipx86chipx86

Indent any {% %} that controls logic flow (for loops, if statements) at indent level 0. Same with the ones …

chipx86chipx86

White space here too, and below on line 7.

mike_conleymike_conley

White space

mike_conleymike_conley

Hm - this could get confusing - there's already a "file" being used outside this loop scope. Is there another …

mike_conleymike_conley

Whitespace in this file too!

mike_conleymike_conley

whitespace

mike_conleymike_conley

I'm a bit wary of having the magic number 9 around. It should probably be assigned to a constant, or …

mike_conleymike_conley

I think we'll want this line indented enough so that the "m" of "max_length" is directly below the "_" of …

mike_conleymike_conley

Constant's are generally assigned to variables in all caps, so this should be: REVISION_PREFIX_COUNT

mike_conleymike_conley

Same as above.

mike_conleymike_conley

While I'm not going to ask you to get rid of all of the trailing whitespace in this file, I …

mike_conleymike_conley

Maybe wrap entire div with the ifequal statement rather than try to hide the div if the count 0.

ME medanat

Add a single indent.

ME medanat

Use class clearfix for this.

ME medanat

Indent a single space.

mike_conleymike_conley
chipx86
  1. Some common style stuff to fix up.
    
    Before we add new fields to the database, I want to be sure we're absolutely going to use them, and that those are more or less set in stone, or it'll be harder to migrate away from it later.
    
    It'd also help if there were unit tests for the new bits of logic.
  2. reviewboard/attachments/models.py (Diff revision 1)
     
     
    Show all issues
    Trailing whitespace.
  3. reviewboard/attachments/models.py (Diff revision 1)
     
     
     
     
    Make sure these are all < 80 chars in length.
  4. reviewboard/attachments/models.py (Diff revision 1)
     
     
    Show all issues
    Do we know that this field is necessary?
  5. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
     
     
     
    Show all issues
    Two blank lines between sections.
    
    The imports must be in alphabetical order.
  6. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
    < 80 columns.
    
    Ideally, one parameter to filter per line.
  7. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
    Show all issues
    Trailing whitespace.
    
    Can you go through the rest of the file and fix those?
  8. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
    < 80 chars.
  9. Show all issues
    Make sure all the HTML uses 1 space indenting.
  10. Show all issues
    Indent any {% %} that controls logic flow (for loops, if statements) at indent level 0.
    
    Same with the ones below.
  11. 
      
JA
JA
mike_conley
  1. Jacob:
    
    Thanks for your work here - just some whitespace that needs to be cleared, and I'm wondering about a magic number that's shown up.
    
    There were two issues that Christian brought up in his last review that haven't been addressed, it seems.  Could you go back and fix those, or comment on why they don't need to be addressed?
    
    Also, could you attach a screenshot showing what this new bit to the diff fragment viewer looks like?
    
    Thanks,
    
    -Mike
  2. Show all issues
    White space here too, and below on line 7.
  3. reviewboard/attachments/models.py (Diff revision 2)
     
     
    Show all issues
    White space
  4. reviewboard/diffviewer/views.py (Diff revision 2)
     
     
    Where is the magic number 9 coming from?
  5. Show all issues
    Hm - this could get confusing - there's already a "file" being used outside this loop scope.  Is there another variable name you could use instead?
  6. 
      
JA
JA
JA
JA
JA
mike_conley
  1. Jacob:
    
    This is looking pretty good - just a few minor notes, thanks.
    
    -Mike
  2. Show all issues
    Whitespace in this file too!
  3. reviewboard/attachments/models.py (Diff revision 4)
     
     
    Show all issues
    whitespace
  4. reviewboard/diffviewer/views.py (Diff revision 4)
     
     
    Show all issues
    I'm a bit wary of having the magic number 9 around.  It should probably be assigned to a constant, or at the very least, there should be a comment here explaining what the 9 is all about.
  5. 
      
JA
JA
mike_conley
  1. Hey Jacob:
    
    Just a few more formatting / style notes.  I've found some lines where the indenting feels a little awkward, but I'm not 100% what the resolution is...maybe Christian, David, or one of the other developers have some input?
    
    Thanks for your work,
    
    -Mike
  2. reviewboard/attachments/models.py (Diff revision 6)
     
     
    Show all issues
    I think we'll want this line indented enough so that the "m" of "max_length" is directly below the "_" of "_(source file revision".
    
    That way, it's easier to tell that those items are within the parentheses following CharField.
  3. reviewboard/attachments/tests.py (Diff revision 6)
     
     
     
    I feel a bit funny about this indentation.  Either the diff_source_revision line should be lined up directly underneath the "diff_source_file" parameter...or, if that causes us to be > 80 chars....I'm not sure.
    
    Not sure what the solution would be there.  :/  At the very least, though, when in doubt, the indenting should be 4 spaces.
    
    Anyone have any input on what the right style would be here?
    1. From pep8 - if you can't line up the second row with the starting delimiter then you should add another line; this code should be 3 lines.
  4. reviewboard/diffviewer/views.py (Diff revision 6)
     
     
    Show all issues
    Constant's are generally assigned to variables in all caps, so this should be:
    
    REVISION_PREFIX_COUNT
  5. reviewboard/diffviewer/views.py (Diff revision 6)
     
     
     
     
    Again, I feel funny about this indentation.  I understand that we want to get these lines < 80 chars, but it makes it harder to tell that these diff_source_x params are within the parentheses following filter.
    
    Christian / David:  do you have a preference?
  6. reviewboard/diffviewer/views.py (Diff revision 6)
     
     
     
     
    Show all issues
    Same as above.
  7. Show all issues
    While I'm not going to ask you to get rid of all of the trailing whitespace in this file, I don't think it's good to add to it either.  The trailing whitespace should be removed from these lines.
    1. sed 's/[ \t]*$//'
      Might make this easy :)
    2. Dave:
      
      Wouldn't that only match spaces followed by tabs? Don't you want to match new lines as well, i.e. "\n". 
      I might be wrong.
  8. Show all issues
    Indent a single space.
  9. 
      
ME
  1. 
      
    1. I think I might have found some HTML style issues. I could be wrong. Be nice if one the mentors could verify those.
      
      -Yazan Medanat
  2. Show all issues
    Maybe wrap entire div with the ifequal statement rather than try to hide the div if the count 0.
  3. Show all issues
    Add a single indent.
  4. Show all issues
    Use class clearfix for this.
  5. 
      
JA
Review request changed
Status:
Discarded