• 
      

    Refactor HTTP access out of HostingService class.

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

    Information

    Review Board
    master
    4d1711b...

    Reviewers

    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
    1. I love this, but I have a couple thoughts on the relationship between the client and service.

    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.

    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. reviewboard/hostingsvcs/service.py (Diff revision 3)
       
       
      Show all issues

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

    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. 
        
    david
    chipx86
    1. Ship It!

    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (7838589).