Add a model representation of a commit in a series

 






 

 


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

 


















 

  

   Information

  





 
 

  brennie
 









 
 

  Review Board
 









 
 

  release-4.0.x
 









 
 

  
 









 
 

  9994
 









 
 

  9631
 









 
 

  b8f56a6...
 






 




 

  

   Reviewers

  





 
 

  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

    
   


  

  

  








 
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

 


 















  

 
















 



 

  

   




brennie




   

   

  

 



 















 



 

  

   




brennie




   

   

  

 



 















 



 

  

   




brennie




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  











 



  

   
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

 


 















  

 
















 



 

  

   




brennie




   

   

  

 



 















 





 

  

   




david




   

   







  

 



 

  


   

   

    
 
david


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  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. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




brennie




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  











 



  

   
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

 


 















  

 
















 



 

  

   




brennie




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/diffviewer/admin.py
      


       (Diff revision 9)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Two blank lines here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  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. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/diffviewer/models/diff_commit.py
      


       (Diff revision 9)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



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

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/diffviewer/models/diff_commit.py
      


       (Diff revision 9)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    These should also have blank=True.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  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.
      
 
      

      2. 


 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/diffviewer/models/diff_commit.py
      


       (Diff revision 9)


      
     
    
    
       
   
    
     
    

    
     
    

    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Same here.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 






  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. 






  9. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This doesn't mention that it may be None.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    10. 






  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>

      
 
      

      2. 


 
    


    

 
    

    11. 




  11. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    12. 




  

 
















 



 

  

   




brennie




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  








 
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

 


 















  

 
















 



 

  

   




brennie




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  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. 






  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. 






  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. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 



 

  

   




brennie




   

   

  

 



 















 



 

  

   




brennie




   

   

  

 



 















 



 

  

   




brennie




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Backtick should be removed.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    This should specify when it might be None.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Missing a docstring.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Do we want > 80 or >= 80?
    
  
    
 
    

 
    
  


    
 
    
 
      

  


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

      2. 


 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/diffviewer/models/mixins.py
      


       (Diff revision 14)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



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

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 






  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.
      
 
      

      2. 


 
    


    

 
    

    8. 




  8. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 




  

 
















 



 

  

   




brennie




   

   

  

 



 















 



 

  

   




brennie




   

   

  

 



 















 





 

  

   




chipx86




   

   





  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




brennie




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  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. 




  3. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  

 
















 



 

  

   




brennie




   

   

  

 



 















 



 

  

   




brennie




   

   

  

 



 















 





 

  

   




chipx86




   

   









  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Should be single quotes.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
      reviewboard/diffviewer/models/mixins.py
      


       (Diff revision 19)


      
     
    
    
       
   
    
     
    

    
     
    

       
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Too many "f"s in "mixin".
    

    Also typo in "collection."
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 




  4. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  

 
















 



 

  

   




brennie




   

   

  

 



 















 



 

  

   




brennie




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  






 

  Status:
  

  Closed (submitted)

  
 




 
Change Summary:

 
Pushed to release-4.0.x (5b09eb0)





 


 










  

 









     

    

   

  

  

   
   Loading...