Summary

Restructure API client

Review Request #4021 — Created April 4, 2013 and submitted April 10, 2013, 2:25 p.m.

Information

Owner

smacleod

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

chipx86, david

Description

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).

Testing Done

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).

Issues

Description	From	Last Updated
Missing a trailing period.	chipx86	April 8, 2013, 3:36 p.m.
Can we maybe swap this to be: if kwargs.pop('internal', True): ... else: ... It's easier to read.	chipx86	April 8, 2013, 3:39 p.m.
Can you fit "self" on the setattr line and the other parameters aligned to that?	chipx86	April 8, 2013, 3:42 p.m.
Can you indent this one more level?	chipx86	April 8, 2013, 3:46 p.m.
Same as above.	chipx86	April 8, 2013, 3:44 p.m.
Same as above.	chipx86	April 8, 2013, 3:46 p.m.
Blank line above.	chipx86	April 8, 2013, 3:46 p.m.
I don't see anything overwriting attribute setting. Can you not just do self._resource = ... ? Failing that, can you …	chipx86	April 8, 2013, 3:48 p.m.
Where does _fields_dict come from? Should this be _fields? If so, can you just do self._fields, like you do below?	chipx86	April 8, 2013, 3:54 p.m.
No need for outer parens.	chipx86	April 8, 2013, 3:54 p.m.
Do you need the ".__iter__" ?	chipx86	April 8, 2013, 3:54 p.m.
Same comments as above.	chipx86	April 8, 2013, 3:55 p.m.
Should end with a period.	chipx86	April 8, 2013, 3:56 p.m.
Should end with a period.	chipx86	April 8, 2013, 3:56 p.m.
HEY! I found the missing "i" !	chipx86	April 8, 2013, 3:59 p.m.
Maybe flip these? Seems more natural to read "If this is an HttpRequest, execute it."	chipx86	April 8, 2013, 3:59 p.m.
embedded	david	April 8, 2013, 10:27 p.m.
Can you group the private variables together?	chipx86	April 8, 2013, 10:30 p.m.
No need for parens.	chipx86	April 8, 2013, 10:33 p.m.
This duplicates all the logic of __getattr__. Maybe we can consolidate them? Maybe just something as simple as: try: return …	chipx86	April 8, 2013, 10:35 p.m.
Should be able to put the first param on the create_resource line and align them all.	chipx86	April 8, 2013, 10:35 p.m.
Can you just do this? return xrange(self.num_items)	david	April 8, 2013, 10:29 p.m.
Shouldn't need parens or +. Or is ReviewBot yelling at you?	chipx86	April 8, 2013, 10:36 p.m.

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:

rbtools/api/decorators.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Missing a trailing period.
```

rbtools/api/decorators.py (Diff revision 1)

The issue has been resolved. Show all issues

Can we maybe swap this to be:

if kwargs.pop('internal', True):
   ...
else:
   ...

It's easier to read.

rbtools/api/resource.py (Diff revision 1)

The issue has been resolved. Show all issues

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

rbtools/api/resource.py (Diff revision 1)

The issue has been resolved. Show all issues

Can you indent this one more level?

smacleod April 8, 2013, 3:59 p.m.

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.

rbtools/api/resource.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same as above.
```
rbtools/api/resource.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same as above.
```
rbtools/api/resource.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line above.
```

rbtools/api/resource.py (Diff revision 1)

The issue has been resolved. 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.

smacleod April 8, 2013, 3:59 p.m.

Ah, this was a leftover from the way things were done previously.

rbtools/api/resource.py (Diff revision 1)

The issue has been resolved. 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?

SM

smacleod April 8, 2013, 3:59 p.m.
```
Ah, missed a spot. Thanks!
```

rbtools/api/resource.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No need for outer parens.
```

rbtools/api/resource.py (Diff revision 1)

The issue has been dropped. Show all issues

Do you need the ".__iter__" ?

chipx86 April 4, 2013, 11:12 p.m.

Ignore this. I wasn't thinking. Not including ".__iter__" woudl just infinitely recurse into this function. Silly me.

rbtools/api/resource.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Same comments as above.
```
rbtools/api/transport/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should end with a period.
```
rbtools/api/transport/__init__.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Should end with a period.
```
rbtools/api/transport/sync.py (Diff revision 1)
The issue has been resolved. Show all issues
```
HEY! I found the missing "i" !
```
1. SM
  
  smacleod April 8, 2013, 3:59 p.m.
  :D

rbtools/api/transport/sync.py (Diff revision 1)

The issue has been resolved. Show all issues

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

Change Summary:

Fixed based on Christian's review. Also added some new unit tests.

Description:

		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 servers 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).
-
-		Based on /r/4004

Diff:

Revision 2 (+384 -492)

Show changes

	rbtools/api/decorators.py
	rbtools/api/factory.py
	rbtools/api/resource.py
	rbtools/api/tests.py
	rbtools/api/transport/__init__.py
	rbtools/api/transport/sync.py

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:

Just a couple small comments.

In your change description, s/no longer servers as/no longer serves as/

smacleod April 8, 2013, 10:29 p.m.

Nice catch, it would have snuck in the commit message :D

rbtools/api/decorators.py (Diff revision 2)
The issue has been resolved. Show all issues
```
embedded
```

rbtools/api/resource.py (Diff revision 2)

The issue has been dropped. Show all issues

Can you just do this?

    return xrange(self.num_items)

chipx86 April 8, 2013, 10:24 p.m.

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.

smacleod April 8, 2013, 10:29 p.m.

This iterator is yielding items from the list, not keys, so I need to do the 'yield self[i]'

rbtools/api/resource.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you group the private variables together?
```
rbtools/api/resource.py (Diff revision 2)
The issue has been resolved. Show all issues
```
No need for parens.
```

rbtools/api/resource.py (Diff revision 2)

The issue has been resolved. 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

smacleod April 8, 2013, 10:36 p.m.

Good catch. I switched the item resources over to this, but missed the ResourceDictField.

rbtools/api/resource.py (Diff revision 2)

The issue has been resolved. Show all issues

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

rbtools/api/tests.py (Diff revision 2)
The issue has been dropped. Show all issues
```
Shouldn't need parens or +. Or is ReviewBot yelling at you?
```
1. SM
  
  smacleod April 8, 2013, 10:36 p.m.
  Yeah, this is for line length.

Change Summary:

Updated based on David's and Christian's reviews.

Description:

		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 servers as an all-wrapping, interface to and
	~	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).

Diff:

Revision 3 (+383 -492)

Show changes

	rbtools/api/decorators.py
	rbtools/api/factory.py
	rbtools/api/resource.py
	rbtools/api/tests.py
	rbtools/api/transport/__init__.py
	rbtools/api/transport/sync.py

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:

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

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