Add the DiffCommit model.
Review Request #6766 — Created Jan. 13, 2015 and discarded
The
DiffSetmodel has been updated to be able to contain several
DiffCommits in addition toFileDiffs. TheDiffCommitmodel
represents a single commit in distributed version control system
(DVCS). EachDiffCommithas one or moreFileDiffs that belong to
it (as well as theDiffSet).Add an evolution to add a foreign key on the
FileDiffmodel to point
to an optionalDiffCommitmodel. Also add an evolution to add a
RelationCounterFieldto theDiffSetmodel that counts the number
of DiffCommits it currently has.Update the
reviewboard.diffviewer.adminmodule to add the ability to
view individualDiffCommits. TheDiffCommits can be as inline
models of theDiffSetmodel and haveFileDiffas an inline model.
Ran unit tests.
Applied evolution successfully.
| Description | From | Last Updated |
|---|---|---|
|
Col: 1 E302 expected 2 blank lines, found 1 |
 reviewbot
|
|
|
Col: 1 W391 blank line at end of file |
reviewbot
|
|
|
Something I realized is that we now have two things called Commit. This one and the one in reviewboard.scmtools.core. Perhaps … |
 chipx86
|
|
|
Should localize the labels using ugettext_lazy. |
chipx86
|
|
|
Let's use commit_type, since type is technically a reserved word. |
chipx86
|
|
|
Is this intended to be the summary, or something else? |
chipx86
|
|
|
I'm not sure the correct answer to this, but I'm wondering if this field is needed, given that it's only … |
chipx86
|
|
|
Maybe commit_id to be consistent with other uses in the codebase. |
chipx86
|
|
|
Maybe worth allowing these to be null/blank? If used in some patchset tool (for, say, CVS/Subversion changes), we may not … |
chipx86
|
|
|
Let's define a constant for the length that we can reuse in all these. COMMIT_ID_LENGTH? |
chipx86
|
|
|
Maybe to make this more useful for debugging/browsing in the admin UI, we can have this include the summary of … |
chipx86
|
|
|
Just a small thing, but it'd be nice to extend this a bit to talk generally about the kinds of … |
chipx86
|
|
|
If you use #: instead of #, we'll be able to include the comments in any auto-generated documentation (which I … |
chipx86
|
|
|
Can this explain what we consider to be a valid commit? |
chipx86
|
|
|
If you make this a @cached_property (a utility provided by Django), then the summary will only ever be calculated once … |
chipx86
|
|
|
It's safe to split on a character that isn't in the string. In that case, you'll just get the entire … |
chipx86
|
|
|
Missing #: |
chipx86
|
|
|
""" on the next line. |
chipx86
|
|
|
We have a mix in the codebase, but I think "Returns" instead of "Gets" does a better job of saying … |
chipx86
|
Change Summary:
Fix PEP8 review.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 2 (+92 -5) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py
Change Summary:
Update description.
Description: |
|
|---|
Change Summary:
Remvoe
commit_ordinalfield. It is going to be removed in the next patch anyway so may aswell do it now.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 3 (+90 -5) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py -
Change Summary:
Remove trailing newline
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 4 (+89 -5) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py
-
Looking good! I have a bunch of comments, but they're minor, or just design questions.
-
reviewboard/diffviewer/models.py (Diff revision 4) Something I realized is that we now have two things called
Commit. This one and the one inreviewboard.scmtools.core.Perhaps (in a separate change) we should rename the other one to
SCMCommit. -
-
reviewboard/diffviewer/models.py (Diff revision 4) Let's use
commit_type, sincetypeis technically a reserved word. -
reviewboard/diffviewer/models.py (Diff revision 4) Is this intended to be the summary, or something else?
-
reviewboard/diffviewer/models.py (Diff revision 4) I'm not sure the correct answer to this, but I'm wondering if this field is needed, given that it's only really used for Subversion, and we have the info in the diffset. Since only RBTools or something custom would be able to post commits, we can probably get away with saying that all commits must have file paths relative to the DiffSet's basedir.
-
reviewboard/diffviewer/models.py (Diff revision 4) Maybe
commit_idto be consistent with other uses in the codebase. -
reviewboard/diffviewer/models.py (Diff revision 4) Maybe worth allowing these to be null/blank? If used in some patchset tool (for, say, CVS/Subversion changes), we may not have this information.
-
reviewboard/diffviewer/models.py (Diff revision 4) Let's define a constant for the length that we can reuse in all these.
COMMIT_ID_LENGTH? -
reviewboard/diffviewer/models.py (Diff revision 4) Maybe to make this more useful for debugging/browsing in the admin UI, we can have this include the summary of the commit message. (Could define a
summary@propertythat grabs the first line fromdescription).
Change Summary:
Address Christian's issues. Remove the
CommitModelreference because it isn't in this patch.
Summary: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Commit: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 5 (+123 -5) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py Tool: Pyflakes Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py
Change Summary:
Add a
RegexValidatorfor commit_id fields in theDiffCommitmodel.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 6 (+130 -5) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py
Change Summary:
Don't use compiled re objects. Add a constant for the regex itself.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 7 (+133 -5) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py Tool: Pyflakes Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py
-
Looks great! I mostly just have some doc suggestions, but I think this is exactly what we want.
(I'll want Steven to have a look as well, since he's been thinking about a lot of this more actively than I.)
-
reviewboard/diffviewer/models.py (Diff revision 7) Just a small thing, but it'd be nice to extend this a bit to talk generally about the kinds of data that can be stored on here and how the commit information is used, just to flesh this out.
-
reviewboard/diffviewer/models.py (Diff revision 7) If you use
#:instead of#, we'll be able to include the comments in any auto-generated documentation (which I really want to start including at some point on the site). -
reviewboard/diffviewer/models.py (Diff revision 7) Can this explain what we consider to be a valid commit?
-
reviewboard/diffviewer/models.py (Diff revision 7) If you make this a
@cached_property(a utility provided by Django), then the summary will only ever be calculated once perCommitinstance. -
reviewboard/diffviewer/models.py (Diff revision 7) It's safe to split on a character that isn't in the string. In that case, you'll just get the entire string.
So, this can be:
text = text.split('\n', 1)[0].strip()
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 8 (+151 -5) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py
Change Summary:
Don't use
^or$in the regex.
Summary: |
|
||||
|---|---|---|---|---|---|
Commit: |
|
||||
Diff: |
Revision 9 (+150 -5) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py Tool: Pyflakes Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py
-
Last three little bits I noticed, and then I'm happy.
-
-
-
reviewboard/diffviewer/models.py (Diff revision 9) We have a mix in the codebase, but I think "Returns" instead of "Gets" does a better job of saying what this will do. A function can get something and store it someplace without ever returning it.
Change Summary:
Address Christian's issues.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 10 (+151 -5) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py
Change Summary:
Allow multiple parents.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 11 (+173 -5) |
-
Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py Tool: Pyflakes Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py
Change Summary:
Forgot to remove
merge_parent_idfield.
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 12 (-4) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/models.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/models.py
-
Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py Tool: Pyflakes Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py
Change Summary:
Rip out datetime fields. When we go to repro the commit, we will stick the header into the extra_data because that stuff can change over time
Commit: |
|
||||
|---|---|---|---|---|---|
Diff: |
Revision 14 (+162 -5) |
-
Tool: Pyflakes Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py
Tool: PEP8 Style Checker Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py Tool: Pyflakes Processed Files: reviewboard/diffviewer/admin.py reviewboard/diffviewer/models.py reviewboard/diffviewer/evolutions/add_commit_history_fields.py reviewboard/diffviewer/evolutions/__init__.py