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).