DiffParser class, subclass for SCM specific diff parse

Review Request #99 — Created June 24, 2007 and submitted

Information

Review Board SVN (deprecated)
trunk

Reviewers

Update:

 - only change is to make the regexp for hunting out binary changes is now a class member variable

================================================================

Update:

These changes preserve the same behaviour of the previous functional approach to diff parsing, it just wraps it all in a class.

 - removed useless whitespace
 - compiled binary-change-matching regexp in __init__

================================================================

Update:

Split diff parsing into smaller methods, makes it easier to tweak per SCM implementation

================================================================

I have split this review request from the GitTool in http://reviews.review-board.org/r/80/

This patch will create a DiffParser class to allow SCMTool's to override the diff parsing operation, and (hopefully) lead to less code duplication. It is pretty much just jamming the previous functional approach into a class.  
Unittests updated, tests pass
chipx86
  1. This change is looking good. I'll probably want David to look over it too, but it looks mainly like code shuffle. No real changes aside from making this extensible, right?
    
    Some small comments. I'll probably look this over in more detail a bit later.
  2. trunk/reviewboard/diffviewer/parser.py (Diff revision 2)
     
     
    Two lines between classes.
    1. Huh, thought I selected that line and down a couple lines to indicate where that was.. But Firefox is gobbling up memory and being laggy, so who knows.
  3. trunk/reviewboard/diffviewer/parser.py (Diff revision 2)
     
     
    Trailing whitespace. There's a lot of this, so can you go through the diff viewer and make sure it's all been removed?
  4. 
      
david
  1. Looks reasonable.  Aside from the cosmetic changes that Christian suggested, my only request is that you compile the regexps and store them in a class data member for the parser.
  2. 
      
david
  1. 
      
  2. trunk/reviewboard/diffviewer/parser.py (Diff revision 4)
     
     
    This can be class data instead of member data, so we only have one for the entire app:
    
    class DiffParser:
        binregexp = re.compile(...)
    
        def __init__(self, data):
            ...
  3. 
      
david
  1. Looks good.  Committing.
  2. 
      
Loading...