Add a model representation of a commit in a series

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

brennie
Review Board
release-4.0.x
9631
fdab7f0...
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.

  • 1
  • 0
  • 22
  • 3
  • 26
Description From Last Updated
Kinda feel like this really should be "commit_count." We also have a "commit" field, and a "file_count," and we should ... chipx86 chipx86
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
Review request changed

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
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. 
      
Loading...