Summary

Move raw patch logic out of view and into diffparser

Review Request #598 — Created Oct. 20, 2008 and discarded

Information

Owner

clach04

Repository

Review Board SVN (deprecated)

Branch

Bugs

Depends On

Reviewers

Groups

reviewboard

People

hanal04

Description

This is mostly to generate discussion. I'd like to implement my own raw patch mechanism (I need to re-create some of the information that was lost during the diff parse stage). I've moved code from the rawview into the diff parser so that I can implement my own logic in a subclass of diffparser.

Moving the logic out of the view also means that the raw patch can be exposed in other places, e.g. we could expose the diff in an email (NOTE if this was done would need to be configurable so that normal users don't suffer the overhead of generating raw patches, e.g. lazy/deferred evaluation or a config option).

A repr() implementation for File also snuck into this change as I found this useful for debugging.

Testing Done

tested with svn (normal raw diff) and with a subclassed custom diffparser

http://reviewboard.googlecode.com/svn/trunk/reviewboard/diffviewer/parser.py (Diff revision 2)
```
Blank line before this.
```

http://reviewboard.googlecode.com/svn/trunk/reviewboard/diffviewer/parser.py (Diff revision 2)

This might be better as:

return repr(self.__dict__)

That way, we get field names, and the hard work is done for us.

clach04 Oct. 27, 2008, 4:07 a.m.

Awesome, I'll remember that for future code :-)

http://reviewboard.googlecode.com/svn/trunk/reviewboard/diffviewer/parser.py (Diff revision 2)

Can you put the comment on the line before this? One "#" and put this in sentence case, end with period.

http://reviewboard.googlecode.com/svn/trunk/reviewboard/diffviewer/parser.py (Diff revision 2)

If we're going to ship this, the docs probably shouldn't talk about how this may be the wrong place for it. We shouldn't add this if we're not sure about it.

I'm unsure as to whether this is the right move. I'm thinking it's not. Rather, if we absolutely need this information, we should just never get rid of it in the first place, and if it makes patch unhappy, then let's have functionality that normalizes a diff before passing it to patch. That can be subclassed in the DiffParser.

clach04 Oct. 27, 2008, 4:12 a.m.

I briefly toyed with the idea of a diffparser that doesn't "loose" data, but I think that would then carry over to File and DiffSet too. I'm not sure we need it yet. The information that is "lost" for my SCM isn't really lost, it is to do with formatting of the file name (file path and version number).

I'm personally more in favor of this approach (a custom raw_diff method) after having spent more time thinking of alternatives. If we really do have an SCM with a custom patch format that would loose data then I think we would have to change the model, I hope we never need to ;-)

http://reviewboard.googlecode.com/svn/trunk/reviewboard/diffviewer/parser.py (Diff revision 3)
```
extra white space should be removed
```

http://reviewboard.googlecode.com/svn/trunk/reviewboard/diffviewer/parser.py (Diff revision 3)

extra white space should be removed

clach04 May 25, 2010, 4:57 p.m.

Apologies - this is for the old svn repo of RB. Once RB was moved to Git I created a new review http://reviews.reviewboard.org/r/1179/ (which is now in the product for I think 1.5). I'll go ahead and mark this review as discarded.