Summary

Add DiffCommit model for review requests with commit histories.

Review Request #6815 — Created Jan. 20, 2015 and submitted Feb. 12, 2015, 11:51 a.m.

Information

Owner

brennie

Repository

Review Board

Branch

master

Bugs

Depends On

Blocks

6933, 6931, 6816, 6934

Commit

aebd320...

Reviewers

Groups

reviewboard

People

chipx86, mike_conley, smacleod

Description

The DiffSet model has been updated to be able to contain several
DiffCommits in addition to FileDiffs. The DiffCommit model
represents a single commit in distributed version control system
(DVCS). Each DiffCommit has one or more FileDiffs that belong to
it (as well as the DiffSet).

Add an evolution to add a foreign key on the FileDiff model to point
to an optional DiffCommit model. Also add an evolution to add a
RelationCounterField to the DiffSet model that counts the number
of DiffCommits it currently has.

Update the reviewboard.diffviewer.admin module to add the ability to
view individual DiffCommits. The DiffCommits can be as inline
models of the DiffSet model and have FileDiff as an inline model.

Add unit tests for creating empty DiffSets as well as creating
DiffCommits from .diff files and uploading via the
UploadDiffCommit form.

Updated the ValidateDiffResource with respect to the new refactoring
of the DiffSetManager.

Testing Done

Unit tests pass.
Applied evolution successfully.

Issues

Description	From	Last Updated
Can you add a docstring to this?	chipx86	Jan. 22, 2015, 8:36 a.m.
User-visible strings typically use double quotes, so the original version of this is preferred. We also try to avoid changing …	chipx86	Jan. 22, 2015, 8:37 a.m.
The labels in these forms have inconsistent casing. For example, "Parent Diff" vs. "Commit type" below. This is not a …	chipx86	Jan. 22, 2015, 8:38 a.m.
This can probably be removed, I think? Looks like the argument defaults and order are all the same as the …	chipx86	Jan. 22, 2015, 8:38 a.m.
While ''' technically works, docstrings traditionally always use """.	chipx86	Jan. 22, 2015, 8:38 a.m.
"Parent commit ID" might be a bit more consistent here.	chipx86	Jan. 22, 2015, 8:39 a.m.
Can you go into some detail on what this is checking?	chipx86	Jan. 22, 2015, 8:42 a.m.
id is a reserved keyword in Python, so we should call this something different. merge_parent_id probably.	chipx86	Jan. 22, 2015, 8:45 a.m.
Should use _(...)	chipx86	Jan. 22, 2015, 8:45 a.m.
ValidationError can take lists of errors and display them appropriately, so you can pass errors directly to it.	chipx86	Jan. 22, 2015, 8:45 a.m.
clean_* functions are expected to return the result. Django will handle updating cleaned_data.	chipx86	Jan. 22, 2015, 8:45 a.m.
Should this be "DiffSetManager"?	chipx86	Jan. 22, 2015, 8:47 a.m.
Instead of referencing the parameter, how about "Creates a list of FileDiffs for a list of files."?	chipx86	Jan. 22, 2015, 8:48 a.m.
When this gets turned into documentation via Sphinx down the road, it's probably going to appear as a paragraph with …	chipx86	Jan. 22, 2015, 8:49 a.m.
The "F" in "parent_diff_File_contents" should be lowercase.	chipx86	Jan. 22, 2015, 8:49 a.m.
If save is False, we'll never actually save any of the merge parents, since they're not returned anywhere. Is this …	chipx86	Jan. 29, 2015, 6:54 a.m.
We can reduce database overhead by using bulk_create. The code will be similar, but you'd create a local instance of …	chipx86	Jan. 22, 2015, 8:52 a.m.
Does this not fit on one line?	chipx86	Jan. 29, 2015, 6:54 a.m.
No blank line needed here.	chipx86	Jan. 22, 2015, 8:53 a.m.
This looks like a constant, but it's really a function. Checking with how Django uses these, I think this should …	chipx86	Jan. 22, 2015, 9:35 a.m.
Should wrap in _(...)	chipx86	Jan. 22, 2015, 9:07 a.m.
Blank line between these.	chipx86	Jan. 29, 2015, 6:55 a.m.
Private functions should go after the public ones.	chipx86	Jan. 29, 2015, 6:58 a.m.
Attributes should go before properties/functions. Same with ones below.	chipx86	Jan. 29, 2015, 7:14 a.m.
Can we add the comment here about the conversion to UTC as well?	chipx86	Jan. 29, 2015, 7 a.m.
Can you add a docstring for this class?	chipx86	Jan. 22, 2015, 9:35 a.m.
This is really an API detail. I think it should be limited to the resources, rather than having the support …	chipx86	Feb. 4, 2015, 1:34 p.m.
Should also go after all public functions.	chipx86	Feb. 4, 2015, 1:32 p.m.
The nose import is old, but both of these should go before the froms, in alphabetical order.	chipx86	Jan. 29, 2015, 7:10 a.m.
'MergeParent' imported but unused	reviewbot	Jan. 22, 2015, 9:35 a.m.
This can easily fit on one line.	chipx86	Jan. 29, 2015, 7:10 a.m.
from reviewboard.diffviewer.models import (DiffCommit, DiffSet, DiffSetHistory, FileDiff)	SM smacleod	Jan. 23, 2015, 12:54 p.m.
This isn't long enough, author will most likely be of the form Firstname LastName <email> and emails can be like …	SM smacleod	Jan. 23, 2015, 12:54 p.m.
Length, same as above.	SM smacleod	Jan. 23, 2015, 12:54 p.m.
Label and help text need to be updated.	SM smacleod	Jan. 23, 2015, 12:51 p.m.
It's not obvious to me why this is a good place to find permissions errors? In fact it seems like …	SM smacleod	Jan. 22, 2015, 2:01 p.m.
Too short	SM smacleod	Jan. 23, 2015, 12:49 p.m.
This should go before the from statements.	chipx86	Jan. 29, 2015, 7:10 a.m.
Col: 1 W391 blank line at end of file	reviewbot	Jan. 29, 2015, 7:17 a.m.
'ValidationError' imported but unused	reviewbot	Jan. 29, 2015, 7:17 a.m.
The raise is over-indented. Missing a %s.	chipx86	Feb. 11, 2015, 8:37 p.m.
Maybe put the comment before the assert.	chipx86	Feb. 11, 2015, 8:38 p.m.
We should have a docstring on this describing its purpose.	chipx86	Feb. 11, 2015, 9:40 p.m.
Over-indentation problems.	chipx86	Feb. 11, 2015, 8:40 p.m.
"Testing", and no trailing period.	chipx86	Feb. 11, 2015, 8:40 p.m.
Too many blank lines.	chipx86	Feb. 11, 2015, 8:41 p.m.
"Testing", and no trailing period.	chipx86	Feb. 11, 2015, 8:41 p.m.
If we're just grabbing the commit_id from each of these, you can probably do: commit.merge_parent_ids.values_list('commit_id', flat=True)	chipx86	Feb. 11, 2015, 8:51 p.m.
'os' imported but unused	reviewbot	Feb. 11, 2015, 10:11 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py

reviewboard/diffviewer/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
 'MergeParent' imported but unused
```

Summary:: Update the diffviewer package.
Add DiffCommit model for review requests with commit histories.

People:: mike_conley

Some new code, so some new comments. Sorry! :) Looking great, though.

reviewboard/diffviewer/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you add a docstring to this?
```

reviewboard/diffviewer/forms.py (Diff revision 1)

The issue has been resolved. Show all issues

User-visible strings typically use double quotes, so the original version of this is preferred. We also try to avoid changing localized strings unless we really need to, to help keep translators from wanting to kill us in our sleep.

reviewboard/diffviewer/forms.py (Diff revision 1)

The issue has been resolved. Show all issues

The labels in these forms have inconsistent casing. For example, "Parent Diff" vs. "Commit type" below. This is not a new problem, but one we should address here.

Looking at other forms, it seems we're more standardized now on sentence casing (e.g., "Parent diff"), so I'd update all the field labels for that.

reviewboard/diffviewer/forms.py (Diff revision 1)

The issue has been resolved. Show all issues

This can probably be removed, I think? Looks like the argument defaults and order are all the same as the parent's, so we'll fall back on that.

reviewboard/diffviewer/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
While ''' technically works, docstrings traditionally always use """.
```
reviewboard/diffviewer/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
"Parent commit ID" might be a bit more consistent here.
```
reviewboard/diffviewer/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you go into some detail on what this is checking?
```

reviewboard/diffviewer/forms.py (Diff revision 1)

The issue has been resolved. Show all issues

id is a reserved keyword in Python, so we should call this something different. merge_parent_id probably.

reviewboard/diffviewer/forms.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should use _(...)
```

reviewboard/diffviewer/forms.py (Diff revision 1)

The issue has been resolved. Show all issues

ValidationError can take lists of errors and display them appropriately, so you can pass errors directly to it.

reviewboard/diffviewer/forms.py (Diff revision 1)

The issue has been resolved. Show all issues

clean_* functions are expected to return the result. Django will handle updating cleaned_data.

reviewboard/diffviewer/managers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should this be "DiffSetManager"?
```

reviewboard/diffviewer/managers.py (Diff revision 1)

The issue has been resolved. Show all issues

Instead of referencing the parameter, how about "Creates a list of FileDiffs for a list of files."?

reviewboard/diffviewer/managers.py (Diff revision 1)

The issue has been resolved. Show all issues

When this gets turned into documentation via Sphinx down the road, it's probably going to appear as a paragraph with lots of dashes inbetween the key names. For that, you'll need to use a ReST-style list, like:

When calling blah blah:

* basedir
* diffset_history
* base_commit_id

reviewboard/diffviewer/managers.py (Diff revision 1)
The issue has been resolved. Show all issues
```
The "F" in "parent_diff_File_contents" should be lowercase.
```

reviewboard/diffviewer/managers.py (Diff revision 1)

The issue has been resolved. Show all issues

We can reduce database overhead by using bulk_create. The code will be similar, but you'd create a local instance of MergeParent (no .objects.create()), put those into a list, and then create them in one go afterward using MergeParent.objects.bulk_create(merge_parents)

reviewboard/diffviewer/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No blank line needed here.
```

reviewboard/diffviewer/models.py (Diff revision 1)

The issue has been dropped. Show all issues

This looks like a constant, but it's really a function. Checking with how Django uses these, I think this should be called validate_commit_id.

Hmm, I also wonder if this should just go in a validators.py, so it's a bit more reusable without needing to reference DiffCommit. What do you think?

brennie Jan. 22, 2015, 9:35 a.m.

Perhaps, but then there is the issue of the COMMIT_ID_RE, which IMO belongs on the DiffCommit class, but if we leave it there we now have a circular import. For now I'm going to leave it on DiffCommit and we can talk about this later.

reviewboard/diffviewer/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should wrap in _(...)
```
reviewboard/diffviewer/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you add a docstring for this class?
```

Change Summary:

Fix issues.

Commit:

0c5cb14621b2bb291857d4a61fb45a93b2e8de44

aebd320826947a5c0d7489274e420e334ab31a23

Diff:

Revision 2 (+612 -112)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py

At about the 75% through this review my brain was just shutting it down. I gave it my best shot but I should probably look over everything again at some point.

reviewboard/diffviewer/admin.py (Diff revision 2)

The issue has been resolved. Show all issues


from reviewboard.diffviewer.models import (DiffCommit,
                                           DiffSet,
                                           DiffSetHistory,
                                           FileDiff)

reviewboard/diffviewer/forms.py (Diff revision 2)

The issue has been resolved. Show all issues

This isn't long enough, author will most likely be of the form Firstname LastName <email> and emails can be like 254 characters alone.

reviewboard/diffviewer/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Length, same as above.
```
reviewboard/diffviewer/forms.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Label and help text need to be updated.
```

reviewboard/diffviewer/managers.py (Diff revision 2)

The issue has been dropped. Show all issues

It's not obvious to me why this is a good place to find permissions errors? In fact it seems like a really strange place?

What am I missing?

brennie Jan. 22, 2015, 2:03 p.m.

This is refactored from somewhere else that wasn't removed from the diff.

reviewboard/diffviewer/models.py (Diff revision 2)

Aside: I know we do this all over the code base, but reading PEP8 today, this is actually frowned upon (including more than one import on the same line like that)

reviewboard/diffviewer/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Too short
```

Change Summary:

Remove some stuff that shouldn't have been in the diff.

Diff:

Revision 3 (+601 -158)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py

Change Summary:

Fix Steven's issues. Split out author/committer fields into name and email fields

Diff:

Revision 4 (+645 -158)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py

Change Summary:

Fix some typos

Diff:

Revision 5 (+647 -158)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py

Change Summary:

DiffCommitForm -> UploadDiffCommitForm

Add author_date and committer_date fields which are backed by _utc and _offset fields that store the time in UTC and the original UTC offset, respectively.

Add UploadDiffCommitFormTests tests.

Description:

		The `DiffSet` model has been updated to be able to contain several
		`DiffCommits` in addition to `FileDiffs`. The `DiffCommit` model
		represents a single commit in distributed version control system
		(DVCS). Each `DiffCommit` has one or more `FileDiffs` that belong to
		it (as well as the `DiffSet`).

		Add an evolution to add a foreign key on the `FileDiff` model to point
		to an optional `DiffCommit` model. Also add an evolution to add a
		`RelationCounterField` to the `DiffSet` model that counts the number
		of `DiffCommits` it currently has.

		Update the `reviewboard.diffviewer.admin` module to add the ability to
		view individual `DiffCommits`. The `DiffCommits` can be as inline
		models of the `DiffSet` model and have `FileDiff` as an inline model.

		Add unit tests for creating empty `DiffSets` as well as creating
~		`DiffCommits` from `.diff` files.
	~	`DiffCommits` from `.diff` files and uploading via the
	+	`UploadDiffCommit` form.

Diff:

Revision 6 (+826 -157)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py

Change Summary:

Use merge and change in lieu of M and C outside of the model.

Diff:

Revision 7 (+825 -157)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py

Change Summary:

Misc validation stuff.

Diff:

Revision 8 (+838 -157)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py

reviewboard/diffviewer/managers.py (Diff revisions 1 - 8)

The issue has been resolved. Show all issues

If save is False, we'll never actually save any of the merge parents, since they're not returned anywhere. Is this okay? If so, we should be sure to document it in the docstring.

reviewboard/diffviewer/models.py (Diff revisions 1 - 8)
The issue has been resolved. Show all issues
```
Does this not fit on one line?
```
reviewboard/diffviewer/models.py (Diff revisions 1 - 8)
The issue has been resolved. Show all issues
```
Blank line between these.
```
reviewboard/diffviewer/models.py (Diff revisions 1 - 8)
The issue has been resolved. Show all issues
```
Private functions should go after the public ones.
```
reviewboard/diffviewer/models.py (Diff revisions 1 - 8)
The issue has been resolved. Show all issues
```
Attributes should go before properties/functions. Same with ones below.
```
reviewboard/diffviewer/models.py (Diff revisions 1 - 8)
The issue has been resolved. Show all issues
```
Can we add the comment here about the conversion to UTC as well?
```

reviewboard/diffviewer/models.py (Diff revisions 1 - 8)

The issue has been resolved. Show all issues

This is really an API detail. I think it should be limited to the resources, rather than having the support here and in the form. Internally, everything else should be solely using the constants.

brennie Jan. 29, 2015, 7:03 a.m.

The uploading form has to support the 'change' and 'merge' values, so should the parsing logic be moved into the form's clean_commit_type method?

I ask because the form is the object that creates the DiffCommit object, via the UploadDiffCommitForm.create method (which mirrors the UploadDiffForm.create method).

For now, I'm going to move the logic into the form (pending further discussion) and leave this issue open.

chipx86 Feb. 4, 2015, 1:33 p.m.
```
Yeah, the form's a good place for this.
```

reviewboard/diffviewer/models.py (Diff revisions 1 - 8)
The issue has been resolved. Show all issues
```
Should also go after all public functions.
```

reviewboard/diffviewer/tests.py (Diff revisions 1 - 8)

The issue has been resolved. Show all issues

The nose import is old, but both of these should go before the froms, in alphabetical order.

reviewboard/diffviewer/tests.py (Diff revisions 1 - 8)
The issue has been resolved. Show all issues
```
This can easily fit on one line.
```
reviewboard/diffviewer/forms.py (Diff revision 8)
The issue has been resolved. Show all issues
```
This should go before the from statements.
```

Diff:

Revision 9 (+841 -160)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py

reviewboard/diffviewer/forms.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Col: 1
 W391 blank line at end of file
```
reviewboard/diffviewer/models.py (Diff revision 9)
The issue has been resolved. Show all issues
```
 'ValidationError' imported but unused
```

Change Summary:

PEP8

Diff:

Revision 10 (+839 -160)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py

Change Summary:

If the committer_date_utc field is set to None, also set the committer_date_offset field to None for consistency.

Diff:

Revision 11 (+844 -160)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py

Change Summary:

Fix typo (underscore to space).

Diff:

Revision 12 (+844 -160)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py

```
Just a few things left.
```
reviewboard/diffviewer/forms.py (Diff revision 12)
The issue has been resolved. Show all issues
```
The raise is over-indented.
Missing a %s.
```
reviewboard/diffviewer/forms.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Maybe put the comment before the assert.
```
reviewboard/diffviewer/models.py (Diff revision 12)
The issue has been resolved. Show all issues
```
We should have a docstring on this describing its purpose.
```
reviewboard/diffviewer/tests.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Over-indentation problems.
```
reviewboard/diffviewer/tests.py (Diff revision 12)
The issue has been resolved. Show all issues
```
"Testing", and no trailing period.
```
reviewboard/diffviewer/tests.py (Diff revision 12)
The issue has been resolved. Show all issues
```
Too many blank lines.
```
reviewboard/diffviewer/tests.py (Diff revision 12)
The issue has been resolved. Show all issues
```
"Testing", and no trailing period.
```

reviewboard/diffviewer/tests.py (Diff revision 12)

The issue has been resolved. Show all issues

If we're just grabbing the commit_id from each of these, you can probably do:

commit.merge_parent_ids.values_list('commit_id', flat=True)

Change Summary:

Rebased on top of latest master. Address Christian's issues.

Description:

		The `DiffSet` model has been updated to be able to contain several
		`DiffCommits` in addition to `FileDiffs`. The `DiffCommit` model
		represents a single commit in distributed version control system
		(DVCS). Each `DiffCommit` has one or more `FileDiffs` that belong to
		it (as well as the `DiffSet`).

		Add an evolution to add a foreign key on the `FileDiff` model to point
		to an optional `DiffCommit` model. Also add an evolution to add a
		`RelationCounterField` to the `DiffSet` model that counts the number
		of `DiffCommits` it currently has.

		Update the `reviewboard.diffviewer.admin` module to add the ability to
		view individual `DiffCommits`. The `DiffCommits` can be as inline
		models of the `DiffSet` model and have `FileDiff` as an inline model.

		Add unit tests for creating empty `DiffSets` as well as creating
		`DiffCommits` from `.diff` files and uploading via the
		`UploadDiffCommit` form.
	+
	+	Updated the `ValidateDiffResource` with respect to the new refactoring
	+	of the `DiffSetManager`.

Diff:

Revision 13 (+844 -162)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.py
	reviewboard/webapi/resources/validate_diff.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/webapi/resources/validate_diff.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/webapi/resources/validate_diff.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py

reviewboard/diffviewer/tests.py (Diff revision 13)
The issue has been resolved. Show all issues
```
 'os' imported but unused
```

Change Summary:

Fixed PEP8

Diff:

Revision 14 (+844 -163)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/forms.py
	reviewboard/diffviewer/managers.py
	reviewboard/diffviewer/models.py
	reviewboard/diffviewer/tests.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/add_commit_history_fields.py
	reviewboard/webapi/resources/validate_diff.py

Tool: Pyflakes
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/webapi/resources/validate_diff.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/diffviewer/admin.py
    reviewboard/diffviewer/models.py
    reviewboard/diffviewer/tests.py
    reviewboard/webapi/resources/validate_diff.py
    reviewboard/diffviewer/evolutions/__init__.py
    reviewboard/diffviewer/forms.py
    reviewboard/diffviewer/evolutions/add_commit_history_fields.py
    reviewboard/diffviewer/managers.py

Ship it!

Woot! Looks good. Ship It!

So we need a branch, both for RB and RBTools. For consistency, they should be the same. While this set of changes are about commit histories, we may want something more general for all the work happening for DVCS-related improvements. Given that, maybe just "dvcs"?

Status:: Completed
Change Summary:: Pushed to dvcs (bc9d45a)