rbtools Python API client

Review Request #3184 - Created July 9, 2012 and submitted

Steven MacLeod
RBTools
master
rbtools
chipx86
This is the base of the new Python API Client. This includes the finished
synchronous transport, and the bare bones resource classes.

In order to support the full Web API, a few resource specific base classes need
to be added (To download diffs etc.). The current state allows for regular use
of the API, accessing resources, following links, and using the uri templates
from the root resource.

Acknowledgement: Alexander Solovets (mbait) provided initial design work, some
starting code, and inspiration for this implementation. His work set me in the
right direction, and is greatly appreciated.

Example Usage:
    from rbtools.api.client import RBClient

    username = None
    password = None
    url = 'http://reviews.reviewboard.org/api/'
    cookie = 'rbapi.cookie'

    client = RBClient(
        url,
        cookie_file=cookie,
        username=username,
        password=password)
    root = client.get_root()

    info = root.get_info()
    print info.product.version

    # Display the first review request.
    requests = root.get_review_requests()
    print requests[0].summary

    # Who are the reviewers?
    for person_link in requests[0].target_people:
        print person_link.get().fullname

    # Lets give a review.
    reviews = requests[0].get_reviews()
    review = reviews.create(
        data={
            'body_top': "Nice patch!",
        })
    review.update(data={'public': True})
Manual testing against local Review Board instance, and the r.rb.org instance. All non-resource-specific operations appear to work.

  • 0
  • 0
  • 39
  • 2
  • 41
Description From Last Updated
Christian Hammond
  1. I know you're looking for higher-level critiques, but I made some notes on stylistic and coding stuff anyway, since I was there and didn't want to forget later.
    
    Higher-level, I think this is looking really good. More docs on some classes would help with figuring out what everything does, but I didn't really feel lost.
    
    I'm excited to see this progress.
    
    One thing that I did want to suggest, as more food for thought, is to hide the root resource. It makes sense to get the root resource as a starting point, but we also already have the client as a staring point. It would be nice to in some way save that step. Maybe keep the fetching of the root in the client, and on the first request, get it, and every subsequent request, proxy through it. More work, but a cleaner API. Not important right now, because the root method works (and it's something that can be done later without breaking the API design).
  2. rbtools/api/client.py (Diff revision 1)
     
     
     
    No space after the """, and in this case, just one line.
    
    There are more docs like this that should be fixed.
  3. rbtools/api/client.py (Diff revision 1)
     
     
     
     
    Just a note that you can do:
    
    # vim: set ts=4 sw=4 expandtab :
  4. rbtools/api/factory.py (Diff revision 1)
     
     
     
     
     
    This can be collapsed into an if/elif/else
  5. rbtools/api/request.py (Diff revision 1)
     
     
    "Parameters"
  6. rbtools/api/resource.py (Diff revision 1)
     
     
     
     
     
     
    Given that there's only one statement after the link check, just do:
    
    if link not in SPECIAL_LINKS:
        setattr(...)
  7. rbtools/api/resource.py (Diff revision 1)
     
     
     
     
     
  8. rbtools/api/resource.py (Diff revision 1)
     
     
     
    Combine these? Or is more coming?
  9. rbtools/api/resource.py (Diff revision 1)
     
     
     
    "to the to the"
    
    "paramaters" -> "parameters"
  10. rbtools/api/resource.py (Diff revision 1)
     
     
    This looks redundant.
  11. rbtools/api/resource.py (Diff revision 1)
     
     
    Is this intentional?
  12. rbtools/api/resource.py (Diff revision 1)
     
     
     
    As an optimization, you can have a regex for {[^}} and use re.sub with a callback function. In this case, just a lambda that returns the value from values. So:
    
        url = self.TEMPLATE_PARAM_RE.sub(lambda m: values[m.group(0)], url)
  13. rbtools/api/transport/__init__.py (Diff revision 1)
     
     
     
    No blank line. Some docs would be nice.
  14. rbtools/api/transport/synch.py (Diff revision 1)
     
     
    The file is synch.py, but we use Sync. I'm thinking we should call the file sync.py.
  15. rbtools/api/transport/synch.py (Diff revision 1)
     
     
    Should be using super(ClassName, self).__setattr__.
  16. rbtools/api/transport/synch.py (Diff revision 1)
     
     
    Can you just use self.__getattribute__?
    1. I think that can cause infinite recursion. If I use my own __getattribute__, my __getattr__ may be called. I think it's best practice to use the object.__getattribute__ when looking up attributes inside these methods.
  17. rbtools/api/transport/synch.py (Diff revision 1)
     
     
    ReviewBot? :)
    
    Just food for thought: It might be nice to make the user agent configurable. Some app out there may want to really identify itself rather than just appear as RBTools.
  18. rbtools/api/transport/synch.py (Diff revision 1)
     
     
     
     
     
     
    Params should be aligned after the (.
  19. 
      
Steven MacLeod
Christian Hammond
  1. 
      
  2. rbtools/api/decode.py (Diff revisions 1 - 2)
     
     
     
     
    Two blank lines.
  3. rbtools/api/resource.py (Diff revisions 1 - 2)
     
     
     
     
    Two blank lines.
  4. rbtools/api/resource.py (Diff revisions 1 - 2)
     
     
    Are these used outside this file?
    1. No, they are only used by the resource class to create the "methods" during __init__. They can't live inside the class since I can't remove them dynamically from instances. Are you thinking a "_" prefix or something?
    2. Yeah, a _ prefix would be good. Just to help signify that they're not part of a public API.
  5. rbtools/api/resource.py (Diff revisions 1 - 2)
     
     
     
    These can be combined.
  6. rbtools/api/resource.py (Diff revisions 1 - 2)
     
     
     
    Probably don't want to wrap the "uri-template-name".
  7. rbtools/api/transport/__init__.py (Diff revisions 1 - 2)
     
     
    resource's
  8. rbtools/api/utils.py (Diff revisions 1 - 2)
     
     
    Docs for this?
  9. rbtools/api/utils.py (Diff revisions 1 - 2)
     
     
    We don't seem to do anything with this.
    1. That was supposed to be 'rindex', which would raise an exception if the '+' was not found. I was kind of abusing the exceptions here. I've re-written without the try block.
  10. rbtools/api/utils.py (Diff revisions 1 - 2)
     
     
    What are we catching?
  11. rbtools/api/transport/sync.py (Diff revision 2)
     
     
    Maybe define these once in this class as a constant.
  12. rbtools/api/transport/sync.py (Diff revision 2)
     
     
     
    Minor thing, but if you flip these two checks (check for 'href' in dict_keys first), it'll be a bit faster. Cheaper check.
  13. rbtools/api/transport/sync.py (Diff revision 2)
     
     
     
    No blank line here.
  14. rbtools/api/transport/sync.py (Diff revision 2)
     
     
    resource's
  15. rbtools/api/transport/sync.py (Diff revision 2)
     
     
    resource's
  16. rbtools/api/transport/sync.py (Diff revision 2)
     
     
     
    I feel like this would be nicer with "self, name, value" on the second line, indented. Same with the one below.
  17. rbtools/api/transport/sync.py (Diff revision 2)
     
     
    Is this a "just in case" exception, or are there any things specifically we're excepting? If the former, we should doc that, and if the latter, we should list them.
    1. Thanks for pointing this out, it made me realize there was a bug in this method.
      
      Also, there is some tricky stuff going on here, which definitely needs to be
      documented, so thanks for mentioning it.
      
      I'm catching the AttributeError exception, because if 
      'resource = object.__getattribute__(self, '_resource')' raises this exception
      it means that self._resource has not been initialized yet, which implies I'm in
      __init__ trying to initialize it or 'self._transport'. In these special cases I
      need to just set the attribute on the object itself, and not look at the fields
      of the resource.
      
      This code is dependent on the ordering of assignments and is a little more
      prone to breakage under modification than I would like. Can you think of a 
      better way to handle this?
    2. Maybe store a _initted flag that we key off of. If set, we set the value directly. Otherwise, we go through the other logic. This would get set as soon as __init__ is done.
  18. rbtools/api/transport/sync.py (Diff revision 2)
     
     
     
    Do we want the number of items on this page, or all? I'd feel like all is more correct, but I'm not 100% sure. Maybe not...
    1. I think we want the number of items on the page, otherwise there would be no way to get this value. I plan to add in support for making the queries with counts only for retrieving the total number.
      
      Any ideas on the interface you'd like to see for this sort of thing?
    2. I thought we had the list resources return the total number as part of the payload? No?
    3. You're right, it is returned. I thought it was originally, but during testing I coincidentally had exactly 25 total items in a list, so I thought my assumption was wrong, and we were returning the page length.
      
      I think __len__ should give the page length, and we could have 'resource.total_results' return the total value?
    4. I think __len__ is actually just too vague in this case. Can we scrap it and have two specific attributes, one for each type of count?
      
      Also, a num_pages or something would be good too, to show how many pages there are in total. This would be calculated based on the number per page and the total results found.
  19. rbtools/api/transport/sync.py (Diff revision 2)
     
     
     
    Is this always correct? Shouldn't this be based on the number of items?
    1. It depends on whether or not we would like a ResourceList with 0 items to be true or false (this makes it True). I thought we might want it to be true since there are still a lot of operations with a list when there are no results, but this isn't really keeping with the way things usually work in Python, so I'll remove this.
  20. rbtools/api/transport/sync.py (Diff revision 2)
     
     
     
    Instead of this and the default value above, just do:
    
    item_content_type = info.get('Item-Content-Type', None)
  21. rbtools/api/transport/sync.py (Diff revision 2)
     
     
    This class and the ones below should, I think, be moved out into a separate file. They're useful outside of the sync transport, and really are their own thing.
    
    (Maybe even more than one file to organize it nicely.)
    1. I'm not sure exactly the best place to put things, or how to split it up, so I shoved them in request.py. Let me know if you have better ideas.
    2. request.py seems fine.
  22. 
      
Steven MacLeod
Steven MacLeod
Steven MacLeod
Steven MacLeod
Steven MacLeod
Christian Hammond
  1. Just about done. A few small nits but they're minor.
  2. rbtools/api/factory.py (Diff revision 6)
     
     
    Should probably be defined once globally. No need to define it every time we create a resource.
  3. rbtools/api/request.py (Diff revision 6)
     
     
     
    Generally, I prefer a list comprehension when possible.
    
    dict([
        (key.replace('-', '_'), value)
        for key, value in args.iteritems()
    ])
  4. rbtools/api/request.py (Diff revision 6)
     
     
     
    Can we do that now? Has the API changed, or can we just do a try/except with the imports at the top of the file?
    1. Yeah I'll take care of this, try/except should be fine.
  5. rbtools/api/resource.py (Diff revision 6)
     
     
     
    Tiny thing, but maybe put the meth(...) on the second line.
  6. rbtools/api/resource.py (Diff revision 6)
     
     
     
     
     
     
    Is there any reason to set this anyway? counts-only is opt-in, so if we don't want it, I don't think we need to set anything.
    1. The reason we need to set this is we are making a request for the 'self' resource from a count, which already has it set. So in the case of this resource self.url = "http://r.rb.org/api/resource/?counts-only=True". This means we need to remove the argument, or override it. The interface for the user has no way to remove arguments, just add and override, so I hadn't added this functionality to HttpRequest yet.
      
      Argument removal code can be tacked on to the HttpRequest without changing any user facing interfaces, so I was focusing on other tasks for now, as we really should fix this on the RB side. The only thing this code chunk affects is trying to call 'resource.get_self()' from a resource that was built with 'get_resource(counts_only=True)'. Counts don't actually have a self link, this was added for convenience.
  7. rbtools/api/resource.py (Diff revision 6)
     
     
     
    Any reason to set token to None? We never do anything with it. I'd think we could set it inline when calling __init__.
    1. This was an attempt to be defensive. We know the Root resource should have token=None, no matter what is guessed by the factory, so I was setting the argument in case there was some code path I didn't anticipate, or a future modification which uses it. Removing this and setting it inline when calling __init__ will have the same effect, but is less durable to future changes.
      
      Remove or keep?
    2. I figured that was the case. It feels a bit odd still, since we're accepting the argument, passing it, but overriding. I understand the need to accept it, as it's an interface and that argument is expected. I don't think that's an unusual thing, and in the other cases, I don't think we follow this pattern.
      
      I would almost say that we should accept **kwargs in the arg list for this and just never let token appear except when setting it when calling super().
    3. I think the **kwargs method would be nicer. I'm trimming down the __init__ arguments for each class to what they need.
  8. rbtools/api/transport/sync.py (Diff revision 6)
     
     
    Should be defined once outside the class. Less work to do each time we instantiate one of these.
    1. This class is only instantiated once per RBClient, but I'll move it out because it is a constant something else might want in the future.
  9. 
      
Steven MacLeod
Christian Hammond
  1. Ship It!
  2. 
      
Steven MacLeod
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to api (d43bd5d)
Loading...