Add original file and patched file resources to the Web API

 






 

 


  Review Request #3008
  —
  Created March 25, 2012 and submitted 

 


















 

  

   Information

  





 
 

  smacleod
 









 
 

  Review Board
 









 
 

  
 









 
 

  
 









 
 

  
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
Adds resources for retrieving the original file, and the patched file from the Web API.

These new resources are children of the File Diff Resource, and provide a way for external tools to retrieve full copies of source files from a review request.

If the file represented by the FileDiffResource is new, the link to the OriginalFileResource will be removed. Alternatively, if the file is deleted, the link to the PatchedFileResource will be removed.

 








  


 
 

  
Tested manually on local dev. Ran unit tests.

 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
Always have the imports listed alphabetically.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
I know the others aren't like this, but "is_new" would feel better to me.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
You can remove the blank line inbetween these.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Combine these. The summary is always right next to the """

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Why not combine these?

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Put the # TODO on the previous line.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Multi-line lists should have each entry on their own line, indented 4 spaces.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Nope.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Summary line.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Not really true. We're only removing links if we're operating on an object. The resource itself may still be a …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Just: "if obj:"

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
I believe the only time that we will have an exception is when something goes very wrong. This means it's …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
In this particular case, we should catch/log per-function, since they're both pretty heavy-weight and we'll end up wanting more detail.

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Probably not necessary. In the context of this function, obj basically means the filediff. Let's just use obj directly.

      		
      
       chipx86chipx86
      
      
       
      
     


    
   

  

 







 






  

 








 














 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    My comments on the first resource apply to the second as well.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/diffviewer/models.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Always have the imports listed alphabetically.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/diffviewer/models.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    I know the others aren't like this, but "is_new" would feel better to me.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I agree. I was a little hesitant naming it new, but decided to follow the previous convention.
      
 
      

      2. 


 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    You can remove the blank line inbetween these.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Combine these. The summary is always right next to the """
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 






  6. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Why not combine these?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 






  7. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Put the # TODO on the previous line.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    8. 






  8. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Multi-line lists should have each entry on their own line, indented 4 spaces.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    9. 






  9. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Nope.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    10. 






  10. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Summary line.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    11. 






  11. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Not really true. We're only removing links if we're operating on an object. The resource itself may still be a list.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I'm not sure I understand how the resource may be a list. "If obj" => "obj is a filediff", which is not a list? If this assumption I'm making is incorrect, then isn't there possibility for breakage with:

filediff = obj

if filediff.new:
      
 
      

      2. 



    2. 
 
 
      
 
      
  
 
      
 
      
  
      A list can serialize each object in it in an "expanded" form. obj would be that entry in the list, rather than the object pertaining to that exact resource URL.

So, for a list of 20 items, get_links may be called 20 times, if the entries are expanded.
      
 
      

      3. 



    3. 
 
 
      
 
      
  
 
      
 
      
  
      That makes sense, I'll fix the comment so It reflects what's actually going on.
      
 
      

      4. 


 
    


    

 
    

    12. 






  12. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/webapi/resources.py
       


        (Diff revision 1)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Just: "if obj:"
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    13. 




  13. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    14. 




  

 
















 



 

  

   





 SM





   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/webapi/resources.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    I believe the only time that we will have an exception is when something goes very wrong. This means it's a problem in Review Board (or a server we talk to) and not a non-existent resource. So, we should return a 500 Internal Server Error instead. We should also log the error we get.

Given that, the except: should be "expect Exception, e:"

Same below.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/webapi/resources.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    In this particular case, we should catch/log per-function, since they're both pretty heavy-weight and we'll end up wanting more detail.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/webapi/resources.py
       


        (Diff revision 2)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Probably not necessary. In the context of this function, obj basically means the filediff. Let's just use obj directly.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 




  5. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  

 
















 



 

  

   





 SM





   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   





 SM





   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  







  

   

    Status:
   

   


    Completed

   

  




  

   

    Change Summary:
   

   

    
Pushed to master (554eaa50414255079e8ff658de05af30e9f12cb9)