Summary

Move raw patch logic out of view and into diffparser

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

Information

Owner

clach04

Repository

Review Board

Branch

release-1.0.5.1

Bugs

Depends On

Reviewers

Groups

reviewboard

People

chipx86

Description

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 http://reviews.reviewboard.org/r/598

Testing Done

reviewboard/diffviewer/parser.py (Diff revision 1)
```
Why do we want this?
```

reviewboard/diffviewer/parser.py (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.
"""

reviewboard/diffviewer/parser.py (Diff revision 1)
```
Can be on one line.
```
reviewboard/diffviewer/parser.py (Diff revision 1)
```
Remove this line.
```
reviewboard/reviews/views.py (Diff revision 1)
```
Does this wrap to < 80 chars?
```

Change Summary:

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

Diff:

Revision 2 (+10 -1)

Show changes

	reviewboard/diffviewer/parser.py
	reviewboard/reviews/views.py

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

clach04 Nov. 13, 2009, 7:47 a.m.

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.

chipx86 Nov. 14, 2009, 5:40 a.m.
```
Fair enough. Looks good then.
```

chipx86 Nov. 14, 2009, 5:41 a.m.

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?

clach04 Nov. 16, 2009, 5:09 a.m.

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!

Ship it!

Looks good. Committed on master (rd71ee46)