Summary

Add a model representation of a commit in a series

Review Request #9609 — Created Feb. 9, 2018 and submitted July 10, 2018, 7:55 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~9994~~

Blocks

9631

Commit

b8f56a6...

Reviewers

Groups

reviewboard

People

Description

This is the first of a series of commits to introduce support for commit
series review. This patch adds the necessary database model
(DiffCommit) and updates the existing models to introduce
relationships.

Testing Done

Ran unit tests.
Applied evolution.

Issues

Description	From	Last Updated
F821 undefined name 'text'	reviewbot	Feb. 9, 2018, 6:33 p.m.
E303 too many blank lines (2)	reviewbot	Feb. 9, 2018, 6:33 p.m.
W292 no newline at end of file	reviewbot	Feb. 9, 2018, 10:35 p.m.
Git commits currently use a 40-character hash, but I'm not sure that's future proof, or even current proof for other …	david	Feb. 21, 2018, 2:08 p.m.
E303 too many blank lines (4)	reviewbot	Feb. 15, 2018, 3:16 p.m.
E303 too many blank lines (2)	reviewbot	Feb. 21, 2018, 2:14 p.m.
Two blank lines here.	chipx86	Feb. 27, 2018, 4:10 p.m.
Can you add doc comments for any of these meant to be public, and prefix any that aren't with a …	chipx86	Feb. 27, 2018, 4:15 p.m.
This might be useful to put in a validators.py.	chipx86	Feb. 27, 2018, 4:17 p.m.
These should also have blank=True.	chipx86	Feb. 27, 2018, 4:18 p.m.
This should describe how the result would look, since it differs based on provided data (and may be None).	chipx86	Feb. 27, 2018, 4:18 p.m.
Same here.	chipx86	Feb. 27, 2018, 4:21 p.m.
Probably worth mentioning in this about the truncation. ... do we want truncation? I almost feel like if we do, …	chipx86	Feb. 27, 2018, 4:23 p.m.
This doesn't mention that it may be None.	chipx86	Feb. 27, 2018, 4:23 p.m.
Worth mentioning, email.utils.formataddr has basically the same logic, with the difference being that an empty e-mail will be in the …	chipx86	Feb. 27, 2018, 4:25 p.m.
F401 'reviewboard.diffviewer.validators.COMMIT_ID_RE' imported but unused	reviewbot	Feb. 27, 2018, 7:41 p.m.
Needs to be one line. These don't need (and probably shouldn't have) literals in the summary. Just say "The maximum …	chipx86	March 9, 2018, 4:09 p.m.
Now that we have summary_truncated, how about getting rid of truncation from summary?	chipx86	March 9, 2018, 4:08 p.m.
I see the need to split this out, but I'm wondering whether this is the right name. How about something …	chipx86	March 9, 2018, 4:08 p.m.
Backtick should be removed.	chipx86	April 16, 2018, 5:48 p.m.
This should specify when it might be None.	chipx86	April 16, 2018, 5:49 p.m.
Missing a docstring.	chipx86	April 16, 2018, 5:49 p.m.
Do we want > 80 or >= 80?	chipx86	April 16, 2018, 5:49 p.m.
This won't link anymore. Needs to be the full path.	chipx86	April 16, 2018, 5:50 p.m.
Why's this removed?	chipx86	April 15, 2018, 4:41 p.m.
Kinda feel like this really should be "commit_count." We also have a "commit" field, and a "file_count," and we should …	chipx86	July 3, 2018, 4 p.m.
Should be single quotes.	chipx86	July 10, 2018, 7:34 p.m.
Too many "f"s in "mixin". Also typo in "collection."	chipx86	July 10, 2018, 7:34 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/models/diff_commit.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F821 undefined name 'text'
```
reviewboard/diffviewer/models/diff_commit.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```

Change Summary:

pep8

Commit:

7da93162016cfef0ef5c94de1168ca3f31e46d8c

49b4d58b7bbd7bba14bee778af0a1fcf7e8ea284

Diff:

Revision 2 (+247 -4)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Refactored out get_total_line_counts into a mixin for inclusion in DiffCommit.

Commit:

49b4d58b7bbd7bba14bee778af0a1fcf7e8ea284

e8da97413e8ccd7f8e56425b50d65ddb757b4d45

Diff:

Revision 3 (+287 -31)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	reviewboard/diffviewer/models/mixins.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/models/mixins.py (Diff revision 3)
The issue has been resolved. Show all issues
```
W292 no newline at end of file
```

Commit:

e8da97413e8ccd7f8e56425b50d65ddb757b4d45

8a8fb4463d44c23c5813f87c71e1ed9f9b1b2714

Diff:

Revision 4 (+287 -31)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	reviewboard/diffviewer/models/mixins.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

8a8fb4463d44c23c5813f87c71e1ed9f9b1b2714

09698fb76131e56cfde691c05b88d1654a78034a

Diff:

Revision 5 (+287 -31)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	reviewboard/diffviewer/models/mixins.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Commit:

09698fb76131e56cfde691c05b88d1654a78034a

f10c8ff324aacd9e54df5392dd187db0724a36fd

Diff:

Revision 6 (+288 -31)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	reviewboard/diffviewer/models/mixins.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/models/diff_commit.py (Diff revision 6)
The issue has been resolved. Show all issues
```
E303 too many blank lines (4)
```

Commit:

f10c8ff324aacd9e54df5392dd187db0724a36fd

6c8036075c2ba280a1f9a83877c118bef4acb5d5

Diff:

Revision 7 (+286 -31)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	reviewboard/diffviewer/models/mixins.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/models/diff_commit.py (Diff revision 5)

The issue has been resolved. Show all issues

Git commits currently use a 40-character hash, but I'm not sure that's future proof, or even current proof for other VCSes. Can we increase the max?

Commit:

6c8036075c2ba280a1f9a83877c118bef4acb5d5

70a9d07bcc97251a272617cdb25b488fa7263b11

Diff:

Revision 8 (+287 -31)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	reviewboard/diffviewer/models/mixins.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/models/diff_commit.py (Diff revision 8)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```

Commit:

70a9d07bcc97251a272617cdb25b488fa7263b11

dbf1718923c96c480d4251466ee492234c7547f9

Diff:

Revision 9 (+286 -31)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	reviewboard/diffviewer/models/mixins.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/admin.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Two blank lines here.
```

reviewboard/diffviewer/models/diff_commit.py (Diff revision 9)

The issue has been resolved. Show all issues

Can you add doc comments for any of these meant to be public, and prefix any that aren't with a _?

reviewboard/diffviewer/models/diff_commit.py (Diff revision 9)
The issue has been resolved. Show all issues
```
This might be useful to put in a validators.py.
```

reviewboard/diffviewer/models/diff_commit.py (Diff revision 9)

The issue has been resolved. Show all issues

These should also have blank=True.

reviewboard/diffviewer/models/diff_commit.py (Diff revision 9)

The issue has been resolved. Show all issues

This should describe how the result would look, since it differs based on provided data (and may be None).

brennie Feb. 27, 2018, 4:26 p.m.

This can't be none because of the guarantee that author_name and author_email are set.

reviewboard/diffviewer/models/diff_commit.py (Diff revision 9)
The issue has been resolved. Show all issues
```
Same here.
```

reviewboard/diffviewer/models/diff_commit.py (Diff revision 9)

The issue has been resolved. Show all issues

Probably worth mentioning in this about the truncation.

... do we want truncation? I almost feel like if we do, it should be its own property.

reviewboard/diffviewer/models/diff_commit.py (Diff revision 9)
The issue has been resolved. Show all issues
```
This doesn't mention that it may be None.
```

reviewboard/diffviewer/models/diff_commit.py (Diff revision 9)

The issue has been dropped. Show all issues

Worth mentioning, email.utils.formataddr has basically the same logic, with the difference being that an empty e-mail will be in the form of name <>. Still, maybe we want to use that instead.

brennie Feb. 27, 2018, 4:26 p.m.

It also puts quotes around the names.

>>> from email.utils import formataddr
>>> print formataddr(('J. Doe', 'jdoe@example.com'))
"J. Doe" <jdoe@example.com>

Change Summary:

Addressed open issues.

Commit:

dbf1718923c96c480d4251466ee492234c7547f9

dcbc9be2f8e2921fdaffebc27958fe581efbd2ab

Diff:

Revision 10 (+309 -31)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	reviewboard/diffviewer/models/mixins.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/models/diff_commit.py (Diff revision 10)
The issue has been resolved. Show all issues
```
F401 'reviewboard.diffviewer.validators.COMMIT_ID_RE' imported but unused
```

Commit:

dcbc9be2f8e2921fdaffebc27958fe581efbd2ab

762d931214eb14e137b379c7921bf52382d0c469

Diff:

Revision 11 (+328 -31)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/validators.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/models/diff_commit.py (Diff revision 11)

The issue has been resolved. Show all issues

Needs to be one line.

These don't need (and probably shouldn't have) literals in the summary. Just say "The maximum length of the e-mail address fields" here, and similar above.

reviewboard/diffviewer/models/diff_commit.py (Diff revision 11)

The issue has been resolved. Show all issues

Now that we have summary_truncated, how about getting rid of truncation from summary?

reviewboard/diffviewer/models/mixins.py (Diff revision 11)

The issue has been resolved. Show all issues

I see the need to split this out, but I'm wondering whether this is the right name. How about something like FileDiffCollectionMixin? We're really talking about something that provides operations for objects that manage filediffs. I could see us expanding this down the road.

In fact, since we're modifying DiffSet anyway, one immediately useful thing I can think of is a RelationCounterField for the files, so that logic doesn't have to query for file countds.

Commit:

762d931214eb14e137b379c7921bf52382d0c469

c93b01fa150880c6dcc28997ccff456dcbeb1909

Diff:

Revision 12 (+334 -32)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/validators.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Fix unit test failures.

Commit:

c93b01fa150880c6dcc28997ccff456dcbeb1909

ad382200a5608b9a7a29f9b1139ea7b92bb789f1

Diff:

Revision 13 (+334 -32)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/validators.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	1 more

Checks run (2 timed out)

flake8 timed out.

JSHint timed out.

Change Summary:

Actually commit changes.

Commit:

ad382200a5608b9a7a29f9b1139ea7b92bb789f1

0793769fdf4b64833749be7fdb1653638dd74097

Diff:

Revision 14 (+334 -33)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/validators.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	2 more

Checks run (2 timed out)

flake8 timed out.

JSHint timed out.

reviewboard/diffviewer/models/diff_commit.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Backtick should be removed.
```
reviewboard/diffviewer/models/diff_commit.py (Diff revision 14)
The issue has been resolved. Show all issues
```
This should specify when it might be None.
```
reviewboard/diffviewer/models/diff_commit.py (Diff revision 14)
The issue has been resolved. Show all issues
```
Missing a docstring.
```
reviewboard/diffviewer/models/diff_commit.py (Diff revision 14)
The issue has been dropped. Show all issues
```
Do we want > 80 or >= 80?
```
1. brennie April 15, 2018, 4:41 p.m.
  Probably > 80 to keep the summary at 80 or less.
reviewboard/diffviewer/models/mixins.py (Diff revision 14)
The issue has been resolved. Show all issues
```
This won't link anymore. Needs to be the full path.
```

reviewboard/reviews/models/review_request.py (Diff revision 14)

The issue has been dropped. Show all issues

Why's this removed?

brennie April 15, 2018, 4:41 p.m.

DiffSets now have a file_count member (which you asked me to add to the mixin). Having both results in an error.

Change Summary:

Addressed Christian's feedback

Commit:

0793769fdf4b64833749be7fdb1653638dd74097

0a9cad7a7a516a8174f0457de3c4cebb665c641f

Diff:

Revision 15 (+339 -33)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/validators.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Rename description -> commit_message from feedback on /r/9642/

Commit:

0a9cad7a7a516a8174f0457de3c4cebb665c641f

da3a490690f9349245f3d23a6d2415325d8a96c3

Diff:

Revision 16 (+339 -33)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/validators.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
Ship It!
```

Change Summary:

In preparation for a change that re-standardizes us on diffset over diff_set.

Commit:

da3a490690f9349245f3d23a6d2415325d8a96c3

fdab7f009dedd90b1b96f0edb4094218b613c4e9

Diff:

Revision 17 (+339 -33)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/validators.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/evolutions/diff_commit_relations.py (Diff revisions 16 - 17)

The issue has been resolved. Show all issues

Kinda feel like this really should be "commit_count." We also have a "commit" field, and a "file_count," and we should be consistent.

Change Summary:

Addressed Christian's feedback.

Commit:

fdab7f009dedd90b1b96f0edb4094218b613c4e9

a1bf88f040dd04501510293633a7d2c185962bce

Diff:

Revision 18 (+339 -33)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/validators.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diff_commit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diff_commit.py
	reviewboard/diffviewer/models/diff_set.py
	reviewboard/diffviewer/models/file_diff.py
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Depends On:

~~9994 - Rename `diff_set` -> `diffset`, etc.~~

Commit:

a1bf88f040dd04501510293633a7d2c185962bce

d37275e4f49e1381888d149bfc6ae9119f6e2db1

Diff:

Revision 19 (+326 -33)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/validators.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diffcommit.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/models/filediff.py
	reviewboard/diffviewer/models/mixins.py
	1 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/diffviewer/models/diffcommit.py (Diff revision 19)
The issue has been resolved. Show all issues
```
Should be single quotes.
```
reviewboard/diffviewer/models/mixins.py (Diff revision 19)
The issue has been resolved. Show all issues
```
Too many "f"s in "mixin".
Also typo in "collection."
```

Change Summary:

Re-add missing evolution

Commit:

d37275e4f49e1381888d149bfc6ae9119f6e2db1

b8f56a6c01fa274cf6010cea29262558b875b784

Diff:

Revision 20 (+340 -33)

Show changes

	reviewboard/diffviewer/admin.py
	reviewboard/diffviewer/validators.py
	reviewboard/diffviewer/evolutions/__init__.py
	reviewboard/diffviewer/evolutions/diffcommit_relations.py
	reviewboard/diffviewer/models/__init__.py
	reviewboard/diffviewer/models/diffcommit.py
	reviewboard/diffviewer/models/diffset.py
	reviewboard/diffviewer/models/filediff.py
	2 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Status:: Completed
Change Summary:: Pushed to release-4.0.x (5b09eb0)