Restructure API client

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

smacleod
RBTools
master
rbtools
chipx86, david
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)
     
     
    Missing a trailing period.
  3. rbtools/api/decorators.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
    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)
     
     
     
    Can you fit "self" on the setattr line and the other parameters aligned to that?
  5. rbtools/api/resource.py (Diff revision 1)
     
     
    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)
     
     
     
    Same as above.
  7. rbtools/api/resource.py (Diff revision 1)
     
     
    Same as above.
  8. rbtools/api/resource.py (Diff revision 1)
     
     
    Blank line above.
  9. rbtools/api/resource.py (Diff revision 1)
     
     
     
    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)
     
     
    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)
     
     
    No need for outer parens.
  12. rbtools/api/resource.py (Diff revision 1)
     
     
    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)
     
     
     
     
     
     
    Same comments as above.
  14. rbtools/api/transport/__init__.py (Diff revision 1)
     
     
    Should end with a period.
  15. rbtools/api/transport/__init__.py (Diff revision 1)
     
     
    Should end with a period.
  16. rbtools/api/transport/sync.py (Diff revision 1)
     
     
    HEY! I found the missing "i" !
  17. rbtools/api/transport/sync.py (Diff revision 1)
     
     
     
     
     
     
     
     
    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)
     
     
    embedded
  3. rbtools/api/resource.py (Diff revision 2)
     
     
     
     
    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)
     
     
     
     
    Can you group the private variables together?
  3. rbtools/api/resource.py (Diff revision 2)
     
     
    No need for parens.
  4. rbtools/api/resource.py (Diff revision 2)
     
     
     
     
     
     
    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)
     
     
     
     
     
     
     
    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)
     
     
     
    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: Closed (submitted)

Change Summary:

Pushed to master and release-0.5.x (bdd336da7da70cc1c9bca8943aeffdb8ab1074a9).
Loading...