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)
     
     
    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)
     
     
     
    There's enough room to fit this on one line.
  4. Must be in the list alphabetically.
  5. reviewboard/reviews/templatetags/reviewtags.py (Diff revision 5)
     
     
     
     
    Two blank lines between top-level functions.
  6. 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. Why can't this be part of the code that generates these?
  8. No space before the ":"
  9. One-space indentation in the HTML files.
  10. the {% should not be indented. Just the contents of it.
  11. No spaces after {{ and before }}.
  12. 
      
AM
  1. 
      
  2. reviewboard/diffviewer/diffutils.py (Diff revision 5)
     
     
    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)
     
     
    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. 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. 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)
     
     
     
     
     
     
    From pep8, don't use "old[-1] != '\n':"
    
    use "not old.endswith('\n'):"
  3. Alphabetical order for loads.
  4. Place those in trans blocks.
  5. reviewboard/templates/reviews/diff_changedesc_field.html (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Single space indentation for html tags.
    Indentation for template tags should be within the {% %}.
    
    i.e.: don't indent the "{%", indent within the "{%" "%}".
  6. 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/
Loading...