Move raw patch logic out of view and into diffparser

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


Review Board SVN (deprecated)


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.

tested with svn (normal raw diff) and with a subclassed custom diffparser
  2. This might be better as:
    return repr(self.__dict__)
    That way, we get field names, and the hard work is done for us.
    1. Awesome, I'll remember that for future code :-)
  3. Can you put the comment on the line before this? One "#" and put this in sentence case, end with period.
  4. 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.
    1. 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 ;-)
  2. extra white space should be removed
  3. extra white space should be removed
    1. Apologies - this is for the old svn repo of RB. Once RB was moved to Git I created a new review (which is now in the product for I think 1.5). I'll go ahead and mark this review as discarded.