Restructure API client

Review Request #4021 — Created April 4, 2013 and submitted

Information

RBTools
master

Reviewers

Restructure API client.

Instead of wrapping everything and hiding resources inside of various
Transport objects, Resource objects are now returned to the user. The
Transport no longer serves as an all-wrapping, interface to and
between everything, super object, but as a simple interface to
the Review Board server.

Resource method calls which return HttpRequests are now decorated,
which serves as the new wrapping mechanism. The decorator captures
calls to these methods, and passes the internal method, and arguments,
off to the Transport to be executed. The new simpler Transport now
only provides a few core methods; primarily it needs to be able to
massage the provided arguments, call the method, and deal with the
returned HttpRequest in a suitable manner. This should make writing
a new Async Transport simple.

Now that proper Resource objects are returned, API users can rely on
checking class types using isinstance to differentiate between item
and list resources etc.

Additionally, dictionary or list fields returned from a resource use
standard class types, regardless of the transport used.

Also, the unit tests have been updated to test the new code, and are
much more sane (reaching into internals much less).
Ran the new unit tests (passed), ran a number of unchanged rbt commands
(including posting this request with 'rbt post'), and used my development
Review Bot instance to test the new code (Review Bot is a user of the api).
Description From Last Updated

Missing a trailing period.

chipx86chipx86

Can we maybe swap this to be: if kwargs.pop('internal', True): ... else: ... It's easier to read.

chipx86chipx86

Can you fit "self" on the setattr line and the other parameters aligned to that?

chipx86chipx86

Can you indent this one more level?

chipx86chipx86

Same as above.

chipx86chipx86

Same as above.

chipx86chipx86

Blank line above.

chipx86chipx86

I don't see anything overwriting attribute setting. Can you not just do self._resource = ... ? Failing that, can you …

chipx86chipx86

Where does _fields_dict come from? Should this be _fields? If so, can you just do self._fields, like you do below?

chipx86chipx86

No need for outer parens.

chipx86chipx86

Do you need the ".__iter__" ?

chipx86chipx86

Same comments as above.

chipx86chipx86

Should end with a period.

chipx86chipx86

Should end with a period.

chipx86chipx86

HEY! I found the missing "i" !

chipx86chipx86

Maybe flip these? Seems more natural to read "If this is an HttpRequest, execute it."

chipx86chipx86

embedded

daviddavid

Can you group the private variables together?

chipx86chipx86

No need for parens.

chipx86chipx86

This duplicates all the logic of __getattr__. Maybe we can consolidate them? Maybe just something as simple as: try: return …

chipx86chipx86

Should be able to put the first param on the create_resource line and align them all.

chipx86chipx86

Can you just do this? return xrange(self.num_items)

daviddavid

Shouldn't need parens or +. Or is ReviewBot yelling at you?

chipx86chipx86
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/api/resource.py
        rbtools/api/transport/__init__.py
        rbtools/api/tests.py
        rbtools/api/factory.py
        rbtools/api/transport/sync.py
        rbtools/api/decorators.py
      Ignored Files:
    
    
  2. 
      
chipx86
  1. 
      
  2. rbtools/api/decorators.py (Diff revision 1)
     
     
    Show all issues
    Missing a trailing period.
  3. rbtools/api/decorators.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
    Show all issues
    Can we maybe swap this to be:
    
    if kwargs.pop('internal', True):
       ...
    else:
       ...
    
    It's easier to read.
  4. rbtools/api/resource.py (Diff revision 1)
     
     
     
    Show all issues
    Can you fit "self" on the setattr line and the other parameters aligned to that?
  5. rbtools/api/resource.py (Diff revision 1)
     
     
    Show all issues
    Can you indent this one more level?
    1. This is the indentation that the pep8 checker is happy with.
      
      If I throw in a set of parens and indent it won't complain,
      and has the readability you're looking for. I'll go with that.
  6. rbtools/api/resource.py (Diff revision 1)
     
     
     
    Show all issues
    Same as above.
  7. rbtools/api/resource.py (Diff revision 1)
     
     
    Show all issues
    Same as above.
  8. rbtools/api/resource.py (Diff revision 1)
     
     
    Show all issues
    Blank line above.
  9. rbtools/api/resource.py (Diff revision 1)
     
     
     
    Show all issues
    I don't see anything overwriting attribute setting. Can you not just do self._resource = ... ?
    
    Failing that, can you do:
    
    self.__dict__.update({
        '_resource': ...,
        '_fields': ...,
    })
    
    I'd prefer that format.
    1. Ah, this was a leftover from the way things were done previously.
  10. rbtools/api/resource.py (Diff revision 1)
     
     
    Show all issues
    Where does _fields_dict come from? Should this be _fields? If so, can you just do self._fields, like you do below?
    1. Ah, missed a spot. Thanks!
  11. rbtools/api/resource.py (Diff revision 1)
     
     
    Show all issues
    No need for outer parens.
  12. rbtools/api/resource.py (Diff revision 1)
     
     
    Show all issues
    Do you need the ".__iter__" ?
    1. Ignore this. I wasn't thinking. Not including ".__iter__" woudl just infinitely recurse into this function. Silly me.
  13. rbtools/api/resource.py (Diff revision 1)
     
     
     
     
     
     
    Show all issues
    Same comments as above.
  14. rbtools/api/transport/__init__.py (Diff revision 1)
     
     
    Show all issues
    Should end with a period.
  15. rbtools/api/transport/__init__.py (Diff revision 1)
     
     
    Show all issues
    Should end with a period.
  16. rbtools/api/transport/sync.py (Diff revision 1)
     
     
    Show all issues
    HEY! I found the missing "i" !
  17. rbtools/api/transport/sync.py (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues
    Maybe flip these? Seems more natural to read "If this is an HttpRequest, execute it."
  18. 
      
SM
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/api/resource.py
        rbtools/api/transport/__init__.py
        rbtools/api/decorators.py
        rbtools/api/factory.py
        rbtools/api/transport/sync.py
        rbtools/api/tests.py
      Ignored Files:
    
    
  2. 
      
david
  1. Just a couple small comments.
    
    In your change description, s/no longer servers as/no longer serves as/
    1. Nice catch, it would have snuck in the commit message :D
  2. rbtools/api/decorators.py (Diff revision 2)
     
     
    Show all issues
    embedded
  3. rbtools/api/resource.py (Diff revision 2)
     
     
     
     
    Show all issues
    Can you just do this?
    
        return xrange(self.num_items)
    1. I did this with a change once and I think that had unexpected results... I've always been curious if that's supposed to be valid or not.
    2. This iterator is yielding items from the list, not keys, so I need to do the 'yield self[i]'
  4. 
      
chipx86
  1. 
      
  2. rbtools/api/resource.py (Diff revision 2)
     
     
     
     
    Show all issues
    Can you group the private variables together?
  3. rbtools/api/resource.py (Diff revision 2)
     
     
    Show all issues
    No need for parens.
  4. rbtools/api/resource.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues
    This duplicates all the logic of __getattr__. Maybe we can consolidate them? Maybe just something as simple as:
    
        try:
            return self.__getattr__(key)
        except AttributeError:
            raise KeyError
    1. Good catch. I switched the item resources over to this, but missed the ResourceDictField.
  5. rbtools/api/resource.py (Diff revision 2)
     
     
     
     
     
     
     
    Show all issues
    Should be able to put the first param on the create_resource line and align them all.
  6. rbtools/api/tests.py (Diff revision 2)
     
     
     
    Show all issues
    Shouldn't need parens or +. Or is ReviewBot yelling at you?
    1. Yeah, this is for line length.
  7. 
      
SM
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        rbtools/api/resource.py
        rbtools/api/transport/__init__.py
        rbtools/api/tests.py
        rbtools/api/factory.py
        rbtools/api/transport/sync.py
        rbtools/api/decorators.py
      Ignored Files:
    
    
  2. 
      
chipx86
  1. Ship It!
  2. 
      
SM
Review request changed
Status:
Completed
Change Summary:
Pushed to master and release-0.5.x (bdd336da7da70cc1c9bca8943aeffdb8ab1074a9).