• 
      

    Implement post-commit review request creation.

    Review Request #4315 — Created July 9, 2013 and submitted

    Information

    Review Board
    master

    Reviewers

    Implement post-commit review request creation.
    
    Our webapi already has the plumbing necessary to create post-commit reviews, and
    in a few previous changes I added the scmtools/hostingsvcs infrastructure for
    fetching the necessary data. This change bridges the gap by making
    update_from_commit_id do the right thing if said commit_id refers to an already
    committed revision on a repository that supports get_change.
    - Ran unit tests.
    - Used this with my UI.
    Description From Last Updated

    """ on the next line.

    chipx86 chipx86

    This is hard to understand without going into the definition. Can you use keyword arguments? Also, create_from_data saves by default.

    chipx86 chipx86

    Col: 80 E501 line too long (80 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (81 > 79 characters)

    reviewbot reviewbot

    Col: 80 E501 line too long (80 > 79 characters)

    reviewbot reviewbot

    Can you verify this works on Python 2.5? We had a property change break a release on 2.5 recently and …

    chipx86 chipx86

    Col: 80 E501 line too long (81 > 79 characters)

    reviewbot reviewbot

    local variable 'diffset' is assigned to but never used

    reviewbot reviewbot
    reviewbot
    1. This is a review from Review Bot.
        Tool: PEP8 Style Checker
        Processed Files:
          reviewboard/reviews/tests.py
          reviewboard/reviews/models.py
        Ignored Files:
      
      
    2. reviewboard/reviews/tests.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (80 > 79 characters)
      
    3. reviewboard/reviews/tests.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (81 > 79 characters)
      
    4. reviewboard/reviews/tests.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (80 > 79 characters)
      
    5. reviewboard/reviews/tests.py (Diff revision 1)
       
       
      Show all issues
      Col: 80
       E501 line too long (81 > 79 characters)
      
    6. 
        
    reviewbot
    1. This is a review from Review Bot.
        Tool: Pyflakes
        Processed Files:
          reviewboard/reviews/tests.py
          reviewboard/reviews/models.py
        Ignored Files:
      
      
    2. 
        
    chipx86
    1. 
        
    2. reviewboard/reviews/models.py (Diff revision 1)
       
       
      Show all issues
      """ on the next line.
    3. reviewboard/reviews/models.py (Diff revision 1)
       
       
       
       
       
      Food for thought: Isn't it possible that the SCMTool may be able to give us a summary and description intelligently without us having to parse? Instead of parsing this here, can Commit just store summary and message fields, and allow them to be set separately, but default to parsing them?
      
      Either way, might be nice to have Commit own this logic here.
      1. I suppose it's theoretically possible, but everything we support now has a single "commit message" so it seems like overkill to duplicate it in every SCMTool. If we run into something that does it differently, we can refactor this later.
        
        I'll move the split logic into Commit for now.
    4. reviewboard/reviews/models.py (Diff revision 1)
       
       
       
       
       
      Show all issues
      This is hard to understand without going into the definition. Can you use keyword arguments?
      
      Also, create_from_data saves by default.
    5. reviewboard/reviews/tests.py (Diff revision 1)
       
       
      Show all issues
      Can you verify this works on Python 2.5? We had a property change break a release on 2.5 recently and I want to be 100% sure.
      1. I'm not set up to run with python2.5 but fget has been part of the object since property() was introduced in 2.2. The regression we had was with .setter, which wasn't introduced until 2.6.
      2. Yeah, just wanted to be sure.
    6. 
        
    david
    reviewbot
    1. This is a review from Review Bot.
        Tool: PEP8 Style Checker
        Processed Files:
          reviewboard/reviews/tests.py
          reviewboard/scmtools/core.py
          reviewboard/reviews/models.py
        Ignored Files:
      
      
    2. 
        
    reviewbot
    1. This is a review from Review Bot.
        Tool: Pyflakes
        Processed Files:
          reviewboard/reviews/tests.py
          reviewboard/scmtools/core.py
          reviewboard/reviews/models.py
        Ignored Files:
      
      
    2. reviewboard/reviews/models.py (Diff revision 2)
       
       
      Show all issues
       local variable 'diffset' is assigned to but never used
      
    3. 
        
    david
    reviewbot
    1. This is a review from Review Bot.
        Tool: PEP8 Style Checker
        Processed Files:
          reviewboard/reviews/tests.py
          reviewboard/scmtools/core.py
          reviewboard/reviews/models.py
        Ignored Files:
      
      
    2. 
        
    reviewbot
    1. This is a review from Review Bot.
        Tool: Pyflakes
        Processed Files:
          reviewboard/reviews/tests.py
          reviewboard/scmtools/core.py
          reviewboard/reviews/models.py
        Ignored Files:
      
      
    2. 
        
    chipx86
    1. Ship It!
    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (8088295).