HostingService API

Review Request #5602 — Created March 9, 2014 and discarded

Information

Review Board
master

Reviewers

API for getting HostingServices and HostingServiceRetositories

tests added to webapi:
tests/test_hosting_service
tests/test_hosting_service_repository,

Description From Last Updated

No blank line here.

chipx86chipx86

Where is this?

chipx86chipx86

Blank line between these.

chipx86chipx86

type is a reserved word. You'll want to use repository_type probably.

chipx86chipx86

Blank line between these. For the setting of the variables, we'd prefer one per line.

chipx86chipx86

Blank line between these.

chipx86chipx86

No space before """

chipx86chipx86

We do this here, but not below or above. Is that intentional?

chipx86chipx86

No blank line here.

chipx86chipx86

No trailing period in test docstrings. Same on all test docstrings below.

chipx86chipx86

You shouldn't use u for strings, since it's implied. Make sure that's fixed everywhere.

chipx86chipx86

Make sure you add the following as the first line, with a blank line below: from __future__ import unicode_literals Same …

chipx86chipx86

Must be in alphabetical order.

chipx86chipx86

Alignment issue.

chipx86chipx86

This class needs a docstring. Note that the docstring is what will go into the actual generated API docs on …

chipx86chipx86

We always use this form: item_child_resources = [ blah, ]

chipx86chipx86

This will also turn into docs on the website. Read through what we say on other resources, and try to …

chipx86chipx86

Should be: list_items = [ {...} for name, class_object in six.iteritems(services) ]

chipx86chipx86

I suggest defining a payload variable in these, and then returning the 200, payload after the conditional.

chipx86chipx86

Alphabetical order.

chipx86chipx86

The first import here is in the wrong group.

chipx86chipx86

I think what we want to do is make this a child of HostingServiceAccountResource, and not HostingServiceResource. By doing this, …

chipx86chipx86

All the same docstring comments and GET <item> apply to this file.

chipx86chipx86

I don't really see a reason to have this function. This can be done inline instead.

chipx86chipx86

If we move this class in the hierarchy, we can also remove this required field, since it'll be part of …

chipx86chipx86

This should be optional here, and should only exist if the service requires it. You can check when we process …

chipx86chipx86

This doesn't mean anything from an API standpoint.

chipx86chipx86

Comments must be in sentence case with a trailing period. Same with all others.

chipx86chipx86

We don't want to supply a dictionary resource. All objects we put in a payload should be the objects themselvs, …

chipx86chipx86

No blank line here.

chipx86chipx86

Each part of a comment should be in sentence casing, with a period at the end. You probably won't need …

chipx86chipx86

Or here.

chipx86chipx86

Should a new error be created in the API errors for when the HostingService throws a PlanError?

OL olessia

Or here.

chipx86chipx86

Alignment is weird. This should be: hosting_service_repository_list_mimetype = \ _build_mimetype('...')

chipx86chipx86

This is third-party, so it should go in the same group as django and djblets.

chipx86chipx86

Shouldn't add this blank line.

chipx86chipx86

A mixin only makes sense if the same set of tests will be used in many places. I don't think …

chipx86chipx86

Should be a single line.

chipx86chipx86

Space after ,

chipx86chipx86

I don't see these being used.

chipx86chipx86

This needs to go by djblets.

chipx86chipx86

This line is too long. Should be more like: return resources....get_list_url( hosting_service=..., local_site_name=...)

chipx86chipx86

How about something like this: if plan not in [pair[0] for pair in self.plans]: That way we aren't adding yet …

daviddavid

The first line of this file should be: from __future__ import unicode_literals

daviddavid

Don't remove this line.

daviddavid

Why did you make this change?

daviddavid

Please add a blank line between these two.

daviddavid

This should use from django.utils import six instead (we want to use the one that they ship, rather than depending …

daviddavid

Please wrap to 80 columns.

daviddavid

Please wrap to 80 columns.

daviddavid

Can you add a blank line between these two?

daviddavid

Please revert this change.

daviddavid

Can you re-wrap this to make it as compact as possible? It might be better to put all args on …

daviddavid

Alignment is off here.

daviddavid

This line should be indented another 4 spaces because there are arguments to get_hosting_service_repository_item_url

daviddavid

Alignment is off here.

daviddavid
OL
OL
chipx86
  1. 
      
  2. reviewboard/hostingsvcs/beanstalk.py (Diff revision 3)
     
     
     

    No blank line here.

  3. reviewboard/hostingsvcs/beanstalk.py (Diff revision 3)
     
     

    Where is this?

  4. reviewboard/hostingsvcs/beanstalk.py (Diff revision 3)
     
     
     

    Blank line between these.

  5. reviewboard/hostingsvcs/beanstalk.py (Diff revision 3)
     
     

    type is a reserved word. You'll want to use repository_type probably.

  6. reviewboard/hostingsvcs/beanstalk.py (Diff revision 3)
     
     
     

    Blank line between these.

    For the setting of the variables, we'd prefer one per line.

  7. reviewboard/hostingsvcs/beanstalk.py (Diff revision 3)
     
     
     

    Blank line between these.

  8. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     

    No space before """

  9. reviewboard/hostingsvcs/github.py (Diff revision 3)
     
     

    We do this here, but not below or above. Is that intentional?

    1. Yes, this adds the authentication token for a logged-in user.

  10. reviewboard/hostingsvcs/repository.py (Diff revision 3)
     
     
     
     

    No blank line here.

  11. reviewboard/hostingsvcs/tests.py (Diff revision 3)
     
     

    No trailing period in test docstrings.

    Same on all test docstrings below.

  12. reviewboard/hostingsvcs/tests.py (Diff revision 3)
     
     
     
     
     
     
     
     
     

    You shouldn't use u for strings, since it's implied. Make sure that's fixed everywhere.

  13. Make sure you add the following as the first line, with a blank line below:

    from __future__ import unicode_literals
    

    Same for every newly introduced file.

  14. reviewboard/webapi/resources/hosting_service.py (Diff revision 3)
     
     
     
     

    Must be in alphabetical order.

  15. Alignment issue.

  16. This class needs a docstring.

    Note that the docstring is what will go into the actual generated API docs on reviewboard.org, so it should be intended for reading by developers trying to learn about the API.

    Also, I don't see any handler for GETing an item on the list?

    1. get_list returns a list of the names of all of the hosting services. I'm not sure what would GETing an item return? What infortmation should be provided on a hosting service, given its name?

    2. Each item should basically contain the same contents as an item in the list returned by get_list.

      So, if get_list returns:

      'hosting_service_accounts': {
          'items': [
              { <payload1> },
              { <payload2> },
          ],
          ...
      }
      

      Each item would be like:

      'hosting_service_account': {
          <payload>
      }
      

      Check out other resources to see how the lists and items differ.

      Basically, every item in a list of resources should be able to be retrieved independently, at a stable URL, so that's what we'd need to provide here. Usually this would all come for free, since most resources are dealing with database objects and all that stuff is automatically done for you, but for custom things like this, you need to do it yourself.

      (I super want to improve this in the Djblets webapi code to be more generic and allow for making usage of more custom payloads easier.. Hey, you wanted a new project, right? ;)

    3. I understand the general principle, but in the specific case of list_items returning only the names of all the hosting services, given a name of a hosting service, say, 'github', what should the paylod be? The name of the the class, i.e. GitHub?

    4. The payload for both the list items and the individual resource should be detailed enough for a new JavaScript interface to have all the info that the Python code currently has, so I'd suggest they both contain at least the following:

      {
          'id': <name>,
          'plans': <plans dictionary or null>,
          'supports_bug_trackers': ...,
          'supports_...': ...,
          'needs_authorization': ...,
          'supported_scmtools': ...,
          'repository_fields': ...,
          'bug_tracker_field': ...,
          'links': {
              'self': {
                  'method': 'GET',
                  'href': <absolute URL to the individual resource>
              },
              'remote_repositories': {
                  'method': 'GET',
                  'href': <absolute URL to your HostingServiceRepositoryResource list for this service>
              },
              'accounts': {
                  'method': 'GET',
                  'href': <absolute URL to the HostingServiceAccountResource list for this service>
              }
          }
      }
      
    5. Should HostingServiceAccount then be a child resource of HostingService?
      Also, based on another comment, remote_repositories is a child resource of the account, so I'm not sure how would I get the links without the account.

  17. We always use this form:

    item_child_resources = [
        blah,
    ]
    
  18. reviewboard/webapi/resources/hosting_service.py (Diff revision 3)
     
     
     
     

    This will also turn into docs on the website. Read through what we say on other resources, and try to come up with some useful text that someone stumbling across the resource will find useful.

  19. Should be:

    list_items = [
        {...}
        for name, class_object in six.iteritems(services)
    ]
    
    1. services is a generator over name, cls in six.iteritems(_hosting_services)

    2. Oh oops.

      In that case, just change the formatting like above. Makes it a bit easier to read when it's on multiple lines, and more consistent with more of the codebase.

  20. reviewboard/webapi/resources/hosting_service.py (Diff revision 3)
     
     
     
     
     

    I suggest defining a payload variable in these, and then returning the 200, payload after the conditional.

  21. Alphabetical order.

  22. The first import here is in the wrong group.

  23. All the same docstring comments and GET <item> apply to this file.

    1. Given that the get_list returns a list of HostingServiceRepository objects that are created from a call to a remote API, what should GET item return? The HostingServiceRepository objects are not stored anywhere. Should this call get_list, and then search through it to return the one repository that was requested?

    2. I think what you should do is have get_list and get both call self.serialize_object on each HostingServiceRepository instance, and have that function handle serializing it to a dictionary.

      get_list would then just iterate over all of them and serialize into a list, while get() would just grab the appropriate one given the values in the URL and serialize just that.

      You'll want the formats of both to be consistent with what a DB-backed resource would return. (Links and ID and stuff.)

  24. I don't really see a reason to have this function. This can be done inline instead.

    1. The reason for doing this is so that I can replace the call to this function with call to _get_remote_repos in the tests to avoid calls to third-party APIS. Please let me know if there's a better way to accomplish this.

    2. Oh I see.

      So, our older tests are just replacing the function on service directly. However, we have this awesome module now called kgb that is designed to safely override functions.

      In your tests, you'd do:

      self.spy_on(service.get_remote_repositories, call_fake=_my_get_remote_repositories)
      

      And then, any time something tries to call service.get_remote_repositories, your function will be called instead.

      See https://github.com/beanbaginc/kgb

    3. I am using kgb =) But I can't replace the function on the service object, because the service object is creted within the API call based on the name of the service that was requested. That's why I have the extra function in the API - in the tests I replace it with a fake call.

    4. I think you can still replace it on the class itself before the call, can you not? We've certainly done this elsewhere.

    5. I tried replacing it on a class, but that only worked if the method was a classmethod.

  25. This doesn't mean anything from an API standpoint.

  26. Comments must be in sentence case with a trailing period.

    Same with all others.

  27. We don't want to supply a dictionary resource. All objects we put in a payload should be the objects themselvs, which will be properly serialized automatically (provided the object is associated with a resource).

    1. Is there an easy way to associate an object that's not a model object with the API?
      So far the best I can come up with for serializing is:

      repos = self._get_remote_repositories(account, plan, service)
      repos = [
                      {self.item_result_key:
                           self.serialize_object(repo, request=request)}
                      for repo in repos
                  ]
      
    2. While the HostingServiceRepository isn't technically a Django model, you should still be able to register it like one, just like we do elsewhere. resources.py should be able to register it to this resource, and then you can provide a custom serialize_object() that properly serializes all the fields we want on it (which won't be __dict__, it'll be all the fields we'd need to identify, represent, and later map to a Repository).

  28. reviewboard/webapi/tests/mimetypes.py (Diff revision 3)
     
     
     

    Alignment is weird. This should be:

    hosting_service_repository_list_mimetype = \
        _build_mimetype('...')
    
  29. reviewboard/webapi/tests/mixins.py (Diff revision 3)
     
     

    This is third-party, so it should go in the same group as django and djblets.

  30. reviewboard/webapi/tests/mixins.py (Diff revision 3)
     
     
     
     

    Shouldn't add this blank line.

  31. reviewboard/webapi/tests/mixins.py (Diff revision 3)
     
     

    A mixin only makes sense if the same set of tests will be used in many places. I don't think that's the case here. These tests should all instead go into the specific unit test.

  32. reviewboard/webapi/tests/mixins.py (Diff revision 3)
     
     
     
     

    Should be a single line.

  33. reviewboard/webapi/tests/test_hosting_service.py (Diff revision 3)
     
     
     
     

    I don't see these being used.

  34. This needs to go by djblets.

  35. reviewboard/webapi/tests/urls.py (Diff revision 3)
     
     

    This line is too long. Should be more like:

    return resources....get_list_url(
        hosting_service=...,
        local_site_name=...)
    
  36. 
      
OL
  1. 
      
  2. Should a new error be created in the API errors for when the HostingService throws a PlanError?

    1. We definitely need errors returned in the API if we encounter them.

      However, that code has some issues that I'll want to point out separately, which will help clarify this question.

  3. 
      
OL
OL
chipx86
  1. 
      
  2. I think what we want to do is make this a child of HostingServiceAccountResource, and not HostingServiceResource.

    By doing this, we already have automatic filtering of account, since that data will be stored on the account.

  3. If we move this class in the hierarchy, we can also remove this required field, since it'll be part of the URL.

  4. This should be optional here, and should only exist if the service requires it. You can check when we process the service below, and return an appropriate error if missing (probably using the MISSING_ATTRIBUTE error).

  5. No blank line here.

  6. Each part of a comment should be in sentence casing, with a period at the end.

    You probably won't need this, though. What you'd want to do, once this is moved in the hierarchy, is to make use of HostingServiceAccountResource.get_object. That should give you the account and its HostingService, and you could follow our other resources to see how we handle those errors.

  7. reviewboard/webapi/tests/mixins.py (Diff revision 3)
     
     

    Space after ,

  8. 
      
OL
david
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     

    How about something like this:

    if plan not in [pair[0] for pair in self.plans]:
    

    That way we aren't adding yet another place where these are enumerated.

  3. reviewboard/hostingsvcs/repository.py (Diff revision 5)
     
     

    The first line of this file should be:

    from __future__ import unicode_literals
    
  4. reviewboard/hostingsvcs/tests.py (Diff revision 5)
     
     

    Don't remove this line.

  5. reviewboard/webapi/base.py (Diff revision 5)
     
     

    Why did you make this change?

    1. HostingServiceRepository is not associated with a model, and trying to serialize it throws AttributeError: 'HostingServiceRepository' object has no attribute 'pk'

  6. Please add a blank line between these two.

  7. This should use from django.utils import six instead (we want to use the one that they ship, rather than depending on another package).

  8. Please wrap to 80 columns.

  9. reviewboard/webapi/resources/hosting_service_repository.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     

    Just a personal preference, but one thing I like doing in situations like these is making the initial condition as simple as possible. In this case, that would mean flipping these to be:

    if plan:
        return INVALID_ATTRIBUTE, ...
    else:
        return MISSING_ATTRIBUTE, ...
    

    If you don't agree, feel free to ignore this. There are more instances below of the same kind of change that you could make.

  10. Please wrap to 80 columns.

  11. Can you add a blank line between these two?

  12. reviewboard/webapi/tests/mixins.py (Diff revision 5)
     
     
     

    Please revert this change.

  13. Can you re-wrap this to make it as compact as possible?

    It might be better to put all args on the next line and indent by 4 spaces.

  14. Alignment is off here.

  15. This line should be indented another 4 spaces because there are arguments to get_hosting_service_repository_item_url

  16. Alignment is off here.

  17. There are also a few things unrelated to the code: * You have 3 issues from previous reviews which are still marked as open, but look like they're fixed in your code. Can you verify and close them? * There's a typo in your description ("HostingServiceRetositories"). Your description should also be fleshed out a lot more. * You've added a .DS_Store file to reviewboard/webapi. Please get rid of this from your change.

OL
OL
Review request changed

Status: Discarded

Loading...