Better Change Description rendering

Review Request #2966 — Created March 17, 2012 and discarded

Information

Review Board

Reviewers

NOTE: this is currently work in progress. I am trying to see if I can modify the style to be more GitHub like

Right now, when a user changes various fields on the review request (only summary and description atm), we show a box that has both the new and old text. It will show what changed in a more usable manner.
Tested in my local linux machine with Chrome using the following steps
1. take a review request, change its description/testing done field
2. publish the change
3. see that the rendering for changes in description/testing done under "Review request changed" have been rendered in an interdiff style accordingly

Screenshots

Description From Last Updated

Two blank lines between top-level methods. There's way too much duplication of this function with the other. Try to split …

chipx86chipx86

There should be a one-line summary of this method on the first line right after '''.

AM ammok

chuck => chunks. missing a period.

AM ammok

There's enough room to fit this on one line.

chipx86chipx86

Must be in the list alphabetically.

chipx86chipx86

Two blank lines between top-level functions.

chipx86chipx86

There should be two lines here.

AM ammok

I'd move this into the changedescs/ app (it's really all about the Change Descriptions, afterall) and name this something else. …

chipx86chipx86

Why can't this be part of the code that generates these?

chipx86chipx86

No space before the ":"

chipx86chipx86

One-space indentation in the HTML files.

chipx86chipx86

No need for the trailing space after class declarations. i.e:

ME medanat

the {% should not be indented. Just the contents of it.

chipx86chipx86

No spaces after {{ and before }}.

chipx86chipx86

From pep8, don't use "old[-1] != '\n':" use "not old.endswith('\n'):"

ME medanat

Alphabetical order for loads.

ME medanat

Place those in trans blocks.

ME medanat

Single space indentation for html tags. Indentation for template tags should be within the {% %}. i.e.: don't indent the …

ME medanat

Loads should be in alphabetical order.

ME medanat
david
  1. Please add some screenshots.
    1. Sure...will do once all my changes are completed
  2. 
      
JI
JI
JI
JI
JI
JI
chipx86
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 5)
     
     
    Show all issues
    Two blank lines between top-level methods.
    
    There's way too much duplication of this function with the other. Try to split the common logic between them out into reusable functions.
    
    diff_line could still be reused, for example. Sure, you won't need the meta information, so that'll just be turned into an optional parameter to the function, but that consolidates a bunch right htere.
    
    Same with new_chunk. Just make these things optional. There are no interesting lines or metadata to go over, so just ensure they become no-ops.
    
    Really, though, anything remaining for ChangeDescription rendering probably belongs in the changedescs/ app, and not here.
  3. reviewboard/diffviewer/diffutils.py (Diff revision 5)
     
     
     
    Show all issues
    There's enough room to fit this on one line.
  4. Show all issues
    Must be in the list alphabetically.
  5. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 5)
     
     
     
     
    Show all issues
    Two blank lines between top-level functions.
  6. Show all issues
    I'd move this into the changedescs/ app (it's really all about the Change Descriptions, afterall) and name this something else. Maybe changedesc_field_diff.
    1. Hmm...I am not sure which file in changedescs/ I should move the function to
  7. Show all issues
    Why can't this be part of the code that generates these?
  8. Show all issues
    No space before the ":"
  9. Show all issues
    One-space indentation in the HTML files.
  10. Show all issues
    the {% should not be indented. Just the contents of it.
  11. Show all issues
    No spaces after {{ and before }}.
  12. 
      
AM
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 5)
     
     
    Show all issues
    There should be a one-line summary of this method on the first line right after '''.
    1. I am not sure what to say here besides "this function returns a new chuck". It is kind of obvious what this function does...
  3. reviewboard/diffviewer/diffutils.py (Diff revision 5)
     
     
    Show all issues
    chuck => chunks.
    missing a period.
  4. reviewboard/diffviewer/diffutils.py (Diff revision 5)
     
     
    Could (a, markup_a) and (b, markup_b) be returned as tuples by some helper function to reduce duplication? Or would it be ill-advised to return tuples like that? (old and new are being modified, and not used later, but it might make sense to keep those changes around so no surprises are introduced by later modifications)
  5. Show all issues
    There should be two lines here.
  6. 
      
ME
  1. Not sure what's been fixed so far, but I have one more minor style issue.
  2. Show all issues
    No need for the trailing space after class declarations.
    
    i.e:
    <col class="line"/>
  3. 
      
JI
JI
JI
ME
  1. A few issues, some that might have been brought up before.
  2. reviewboard/diffviewer/diffutils.py (Diff revision 7)
     
     
     
     
     
     
    Show all issues
    From pep8, don't use "old[-1] != '\n':"
    
    use "not old.endswith('\n'):"
  3. Show all issues
    Alphabetical order for loads.
  4. Show all issues
    Place those in trans blocks.
  5. reviewboard/templates/reviews/diff_changedesc_field.html (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Single space indentation for html tags.
    Indentation for template tags should be within the {% %}.
    
    i.e.: don't indent the "{%", indent within the "{%" "%}".
  6. Show all issues
    Loads should be in alphabetical order.
  7. 
      
JI
JI
Review request changed
Status:
Discarded
Change Summary:
Obsoleted in favor of https://reviews.reviewboard.org/r/5499/