Move raw patch logic out of view and into diffparser

Review Request #1179 — Created Oct. 23, 2009 and submitted


Review Board


Moves "create diff/patch" logic out of the view and into the diffparser. This way the view just deals with "strings" rather than having to create diff fragments.

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).

Related to old (svn) review

  2. reviewboard/diffviewer/ (Diff revision 1)
    Why do we want this?
  3. reviewboard/diffviewer/ (Diff revision 1)
    Probably something more like:
    """Returns a raw diff as a string.
    The returned diff as composed of all FileDiffs in the provided diffset.
  4. reviewboard/diffviewer/ (Diff revision 1)
    Can be on one line.
  5. reviewboard/diffviewer/ (Diff revision 1)
    Remove this line.
  6. reviewboard/reviews/ (Diff revision 1)
    Does this wrap to < 80 chars?
Review request changed

Change Summary:

* doc string updated (took suggested review comment text)
* removed diagnostic code that ended up in diff
* broke long line into two statements


Revision 2 (+10 -1)

Show changes

  1. So the change looks fine, style-wise, but I guess the thing I'm a little unsure about is the placement of the raw_diff function. It seems perhaps wrong to put it in DiffParser, a class responsible for the parsing of diffs. I don't have a better place off-hand to suggest, except maybe just in SCMTool (which is growing larger than I'd like, but it may be more appropriate there).
    1. With the current model, DiffParser is the only place to put it as it is the only class that knows about manipulating diffs; either parsing or reconstituting.
      SCMTool does have some knowledge of diffs BUT this is based on DiffParser via the SCMTool.get_parser() function/method. Many of the SCM types then go ahead and subclass DiffParser into an SCM specific one, e.g. CVSDiffParser, PerforceDiffParser, etc. SCMTool should not (and does not currently) have any responsibility for diffs other than which diff parser to use.
      I too have a custom DiffParser subclass and I've implemented an SCM specific raw_diff function in that subclass.
    2. Fair enough. Looks good then.
    3. I know the development was done on 1.0.x, but this will probably end up in 1.1. We're trying to limit 1.0.x to important bugs. Will this impact you too negatively?
    4. No that's fine. I'll try and plan a 1.1 roll out at some point. I'm not yet sure if I'm going to be brave and rollout a release candidate or not though. Thanks for accepting the patch!
  1. Looks good. Committed on master (rd71ee46)