If the summary is more than 300 chars Trim it down

Review Request #482 — Created July 31, 2008 and submitted

Information

Review Board SVN (deprecated)
515

Reviewers

When using post-review with perforce there is an error if the CL description has more that 300 chars without a newline. A SQL warning is generated (truncating summary) that will cause post0review to throw an error 500. There is an example of the error page in Bug 515. This is my first attempt in Python so the code may not be the best. I created a function that will truncate a string at the first '.' or after the 300th char.
I submitted and updated a review that had more than 300 chars without a \n and a '.'.
I submitted and updated a review that had more than 300 chars without a \n and did not have a '.' 
I submitted and updated a review that had more than 300 chars without a \n and had '.' after the 300th char 
JS
  1. 
      
  2. /trunk/reviewboard/reviews/models.py (Diff revision 1)
     
     
    I for got to take out my debut messages. I will remove them in the morning
  3. 
      
chipx86
  1. I started to review this but then I began to think about what we want to do here. I'm not sure truncating like this is really the desired behavior. We should be limiting the size in the web UI (which I believe we do) and we should probably provide a validation error to clients using the web API that set a value that's too long. It's then up to the calling program to limit the size.
    
    So the truncation logic should probably go in post-review.
    1. We'd need to handle this in our perforce changeset description parser, too (bug 438)
    2. I made the requested changed.  I tried doing this in post-review first but it would still add the full description to it.  I will look at it again today and see if I can fix this in post-review 
  2. /trunk/reviewboard/reviews/models.py (Diff revision 1)
     
     
    This would be "truncate".
  3. /trunk/reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
     
     
    All this file stuff should go away.
  4. /trunk/reviewboard/reviews/models.py (Diff revision 1)
     
     
    Trailing whitespace. Also in other places in the change.
  5. 
      
david
  1. 
      
  2. /trunk/reviewboard/reviews/models.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    This algorithm isn't quite right. Two problems with it:
    1. If the first sentence is longer than 300 characters, it'll still leave a string too long.
    2. It would be nice to include as many sentences as possible within the allowed 300 characters instead of just the first.
    1. Please see the updated diff
  3. /trunk/reviewboard/reviews/models.py (Diff revision 2)
     
     
    One space after ,s
  4. /trunk/reviewboard/reviews/models.py (Diff revision 2)
     
     
    One space after ,s
  5. 
      
david
  1. 
      
  2. /trunk/reviewboard/reviews/models.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    This is way more complicated than it needs to be.  Here's what I suggest:
    
    def truncate(string, n):
        if len(string) > n:
            string = string[0:n]
            i = string.rfind('.')
            if i != -1:
                string = string[0:i + 1]
        return string
    
    1. Made these changes.  I had no idea that python could be so simple.
  3. 
      
CF
  1. 
      
  2. /trunk/reviewboard/reviews/models.py (Diff revision 3)
     
     
    Is is possible to pull the number 300 from a config file? Would be nice to avoid magic numbers in the code... just my $0.02
    1. I don't feel this is something worth customizing, but it should be a constant in the file.
    2. 300 is the limit of the field in the model, not some value we picked out of a hat.
      
      A comment to this effect probably wouldn't hurt.
    3. Added the comment and global variable
  3. 
      
CF
  1. 
      
  2. /trunk/reviewboard/reviews/models.py (Diff revision 3)
     
     
    No doubt, obviously not trying to start a flame-war or nit-pick session. Commenting is a great idea, especially if you factor out the 300 into a global or class variable like "max_summary_length":
    
    /* truncate summary to avoid p4 error - experimental limit is 300 chars */
    self.summary = truncate(self.summary, max_summary_length)
  3. 
      
david
  1. Committed as r1459.  Thanks!
  2. 
      
Loading...