Add a model representation of a commit in a series

Review Request #9609 — Created Feb. 9, 2018 and submitted

Information

Review Board
release-4.0.x
b8f56a6...

Reviewers

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.

Ran unit tests.
Applied evolution.

Description From Last Updated

F821 undefined name 'text'

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

Git commits currently use a 40-character hash, but I'm not sure that's future proof, or even current proof for other …

daviddavid

E303 too many blank lines (4)

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

Two blank lines here.

chipx86chipx86

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

chipx86chipx86

This might be useful to put in a validators.py.

chipx86chipx86

These should also have blank=True.

chipx86chipx86

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

chipx86chipx86

Same here.

chipx86chipx86

Probably worth mentioning in this about the truncation. ... do we want truncation? I almost feel like if we do, …

chipx86chipx86

This doesn't mention that it may be None.

chipx86chipx86

Worth mentioning, email.utils.formataddr has basically the same logic, with the difference being that an empty e-mail will be in the …

chipx86chipx86

F401 'reviewboard.diffviewer.validators.COMMIT_ID_RE' imported but unused

reviewbotreviewbot

Needs to be one line. These don't need (and probably shouldn't have) literals in the summary. Just say "The maximum …

chipx86chipx86

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

chipx86chipx86

I see the need to split this out, but I'm wondering whether this is the right name. How about something …

chipx86chipx86

Backtick should be removed.

chipx86chipx86

This should specify when it might be None.

chipx86chipx86

Missing a docstring.

chipx86chipx86

Do we want > 80 or >= 80?

chipx86chipx86

This won't link anymore. Needs to be the full path.

chipx86chipx86

Why's this removed?

chipx86chipx86

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

chipx86chipx86

Should be single quotes.

chipx86chipx86

Too many "f"s in "mixin". Also typo in "collection."

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

brennie
brennie
Review request changed
Change Summary:

Refactored out get_total_line_counts into a mixin for inclusion in DiffCommit.

Commit:
49b4d58b7bbd7bba14bee778af0a1fcf7e8ea284
e8da97413e8ccd7f8e56425b50d65ddb757b4d45

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
brennie
brennie
Review request changed
brennie
david
  1. 
      
  2. 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?

  3. 
      
brennie
Review request changed
brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/admin.py (Diff revision 9)
     
     
     
     
    Show all issues

    Two blank lines here.

  3. reviewboard/diffviewer/models/diff_commit.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

  4. reviewboard/diffviewer/models/diff_commit.py (Diff revision 9)
     
     
     
     
    Show all issues

    This might be useful to put in a validators.py.

  5. reviewboard/diffviewer/models/diff_commit.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    These should also have blank=True.

  6. Show all issues

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

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

  7. reviewboard/diffviewer/models/diff_commit.py (Diff revision 9)
     
     
     
     
    Show all issues

    Same here.

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

  9. Show all issues

    This doesn't mention that it may be None.

  10. reviewboard/diffviewer/models/diff_commit.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
    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.

    1. It also puts quotes around the names.

      >>> from email.utils import formataddr
      >>> print formataddr(('J. Doe', 'jdoe@example.com'))
      "J. Doe" <jdoe@example.com>
      
  11. 
      
brennie
Review request changed
Change Summary:

Addressed open issues.

Commit:
dbf1718923c96c480d4251466ee492234c7547f9
dcbc9be2f8e2921fdaffebc27958fe581efbd2ab

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/models/diff_commit.py (Diff revision 11)
     
     
     
    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.

  3. reviewboard/diffviewer/models/diff_commit.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

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

  4. reviewboard/diffviewer/models/mixins.py (Diff revision 11)
     
     
    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.

  5. 
      
brennie
brennie
brennie
chipx86
  1. 
      
  2. Show all issues

    Backtick should be removed.

  3. Show all issues

    This should specify when it might be None.

  4. Show all issues

    Missing a docstring.

  5. Show all issues

    Do we want > 80 or >= 80?

    1. Probably > 80 to keep the summary at 80 or less.

  6. reviewboard/diffviewer/models/mixins.py (Diff revision 14)
     
     
    Show all issues

    This won't link anymore. Needs to be the full path.

  7. reviewboard/reviews/models/review_request.py (Diff revision 14)
     
     
     
     
    Show all issues

    Why's this removed?

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

  8. 
      
brennie
brennie
chipx86
  1. Ship It!

  2. 
      
brennie
chipx86
  1. 
      
  2. 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.

  3. 
      
brennie
brennie
chipx86
  1. 
      
  2. Show all issues

    Should be single quotes.

  3. reviewboard/diffviewer/models/mixins.py (Diff revision 19)
     
     
    Show all issues

    Too many "f"s in "mixin".

    Also typo in "collection."

  4. 
      
brennie
brennie
Review request changed
Status:
Completed
Change Summary:
Pushed to release-4.0.x (5b09eb0)