Refactor HTTP access out of HostingService class.

 






 

 


  Review Request #5691
  —
  Created April 9, 2014 and submitted 

 


















 

  

   Information

  





 
 

  david
 









 
 

  Review Board
 









 
 

  master
 









 
 

  
 









 
 

  
 









 
 

  4d1711b...
 






 




 

  

   Reviewers

  





 
 

  reviewboard
 









 
 

  
 






 








 





  


 
 

  
This change adds a new HostingServiceClient class which handles the basic

details of HTTP and JSON communication with hosting service APIs. This is

primarily just extracting the functionality out of the HostingService class.

Individual HostingServices can override this class to add additional checking

(such as GitHub's checking of rate limit headers), or add higher-level API

functionality.

 








  


 
 

  
    

  • Ran unit tests.
    • 

  • Tested post-commit review request creation.
    • 


 







 









 





  








 






 
 
 

  

   

    

     
    

    
    
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
     
  • 
      
     
    • 
    

   

   

    
     

      Description
      From
      Last Updated
     

    
    



     

      
       
       
I think there may be some ways we can improve the relationship between the client and service here. Right now, …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
Missing trailing period. It'd also be nice to flesh this out more and say what its responsibilities are, and that …

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
While we're here, can we add some docstrings to these methods?

      		
      
       chipx86chipx86
      
      
       
      
     




     

      
       
       
With the above, this would just be: self.client = self.build_client(service=self)

      		
      
       chipx86chipx86
      
      
       
      
     


    
   

  

 







 






  

 








 














 



 

  

   




david




   

   

  

 



 















 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    I love this, but I have a couple thoughts on the relationship between the client and service.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 

















  2. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/hostingsvcs/github.py
       


        (Diff revision 3)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    I think there may be some ways we can improve the relationship between the client and service here. Right now, HostingService is either given a client, or will construct one by itself, and having an account requires doing a subclass and constructing it and passing it to the HostingService's constructor.
    

    I'd propose we change this design a bit to ensure client construction happens in a more consistent way. I'd suggest doing this by having a build_client method on HostingService that constructs a HostingServiceClient by default, passing in *args, **kwargs to it. HostingService.__init__ would set self.client to this, passing service=self as a parameter (allowing any client to access the parent service and therefore account).
    

    GitHubClient would then just override build_client to return an instance of GitHubClient instead. It wouldn't have to do anything special with the account, and would just pass the arguments it's supposed to pass.
    

    This makes it easier for us to add additional default parameters down the road and to keep HostingService subclasses clean.
    
  
    
 
    

 
    
  


    
 
    
 
      

  


    1. 
 
 
      
 
      
  
 
      
 
      
  
      I'm going to do something related but different. Given that the HostingService should have any data that the HostingServiceClient might need, we can just pass that in as a parameter to all the client __init__s, and then subclasses can pull out what they need. Then I'll just define the client class as a parameter in the HostingService.
      
 
      

      2. 


 
    


    

 
    

    3. 






  3. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/hostingsvcs/service.py
       


        (Diff revision 3)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    Missing trailing period.
    

    It'd also be nice to flesh this out more and say what its responsibilities are, and that it can be subclassed for the same reasons you outline in the review request's description.
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    4. 






  4. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/hostingsvcs/service.py
       


        (Diff revision 3)


       
      
     
    
        
    
     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    While we're here, can we add some docstrings to these methods?
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    5. 






  5. 
 
    
  
  
    
  
 
    
  
    
   
    
    
     
    
      
       reviewboard/hostingsvcs/service.py
       


        (Diff revision 3)


       
      
     
    
        
    
     
     
    

     
     
    

     
     
    

     
     
    

        
   
    
  
    
 
    

   
    
    
    




    
 
    
  
  
  

  
  
   Show all issues
  
 
    

    



    
    With the above, this would just be:
    

    self.client = self.build_client(service=self)

    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    6. 




  6. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    7. 




  

 
















 



 

  

   




david




   

   

  

 



 















 





 

  

   




chipx86




   

   







  

 



 

  


   

   

    
 
chipx86


    
   


  

  

  



    




  1. 
 
    
  
    
   
    Ship It!
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    2. 















  2. 
 
    
  
    
   
    
  
    
 
    

 
    
  


    
 
    
 
      

  

 
    


    

 
    

    3. 




  

 
















 



 

  

   




david




   

   

  

 



 

  


   

   

    
 Review request changed

    
   


  

  

  







  

   

    Status:
   

   


    Completed

   

  




  

   

    Change Summary:
   

   

    
Pushed to master (7838589).