Add a model representation of a commit in a series

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

brennie
Review Board
release-4.0.x
9994
9631
b8f56a6...
reviewboard

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    Two blank lines here.

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

    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)
     
     
     
     

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

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

    These should also have blank=True.

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

    Same here.

  8. 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. This doesn't mention that it may be None.

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

    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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

brennie
chipx86
  1. 
      
  2. reviewboard/diffviewer/models/diff_commit.py (Diff revision 11)
     
     
     

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

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

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

    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. Backtick should be removed.

  3. This should specify when it might be None.

  4. Missing a docstring.

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

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

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

    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. 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. Should be single quotes.

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

    Too many "f"s in "mixin".

    Also typo in "collection."

  4. 
      
brennie
brennie
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (5b09eb0)
Loading...