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)
     
     
     
    Show all issues

    No blank line here.

  3. reviewboard/hostingsvcs/beanstalk.py (Diff revision 3)
     
     
    Show all issues

    Where is this?

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

    Blank line between these.

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

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

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

    Blank line between these.

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

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

    Blank line between these.

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

    No space before """

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

    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)
     
     
     
     
    Show all issues

    No blank line here.

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

    No trailing period in test docstrings.

    Same on all test docstrings below.

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

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

  13. Show all issues

    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)
     
     
     
     
    Show all issues

    Must be in alphabetical order.

  15. Show all issues

    Alignment issue.

  16. Show all issues

    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. Show all issues

    We always use this form:

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

    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. Show all issues

    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)
     
     
     
     
     
    Show all issues

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

  21. Show all issues

    Alphabetical order.

  22. Show all issues

    The first import here is in the wrong group.

  23. Show all issues

    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. Show all issues

    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. Show all issues

    This doesn't mean anything from an API standpoint.

  26. Show all issues

    Comments must be in sentence case with a trailing period.

    Same with all others.

  27. Show all issues

    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)
     
     
     
    Show all issues

    Alignment is weird. This should be:

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

    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)
     
     
     
     
    Show all issues

    Shouldn't add this blank line.

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

    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)
     
     
     
     
    Show all issues

    Should be a single line.

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

    I don't see these being used.

  34. Show all issues

    This needs to go by djblets.

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

    This line is too long. Should be more like:

    return resources....get_list_url(
        hosting_service=...,
        local_site_name=...)
    
  36. 
      
OL
  1. 
      
  2. Show all issues

    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. Show all issues

    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. Show all issues

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

  4. Show all issues

    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. Show all issues

    No blank line here.

  6. Show all issues

    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. Show all issues

    Or here.

  8. Show all issues

    Or here.

  9. reviewboard/webapi/tests/mixins.py (Diff revision 3)
     
     
    Show all issues

    Space after ,

  10. 
      
OL
david
  1. 
      
  2. reviewboard/hostingsvcs/github.py (Diff revision 5)
     
     
    Show all issues

    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)
     
     
    Show all issues

    The first line of this file should be:

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

    Don't remove this line.

  5. reviewboard/webapi/base.py (Diff revision 5)
     
     
    Show all issues

    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. Show all issues

    Please add a blank line between these two.

  7. Show all issues

    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. Show all issues

    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. Show all issues

    Please wrap to 80 columns.

  11. Show all issues

    Can you add a blank line between these two?

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

    Please revert this change.

  13. Show all issues

    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. Show all issues

    Alignment is off here.

  15. Show all issues

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

  16. Show all issues

    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