-
-
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.
-
rbtools/api/client.py (Diff revision 1) Just a note that you can do: # vim: set ts=4 sw=4 expandtab :
-
-
-
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(...)
-
-
-
-
-
-
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)
-
-
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.
-
rbtools/api/transport/synch.py (Diff revision 1) Should be using super(ClassName, self).__setattr__.
-
-
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.
-
rbtools Python API client
Review Request #3184 — Created July 9, 2012 and submitted
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.
Description | From | Last Updated |
---|---|---|
No space after the """, and in this case, just one line. There are more docs like this that should … |
chipx86 | |
Two blank lines. |
chipx86 | |
This can be collapsed into an if/elif/else |
chipx86 | |
"Parameters" |
chipx86 | |
Two blank lines. |
chipx86 | |
Given that there's only one statement after the link check, just do: if link not in SPECIAL_LINKS: setattr(...) |
chipx86 | |
Same. |
chipx86 | |
Combine these? Or is more coming? |
chipx86 | |
"to the to the" "paramaters" -> "parameters" |
chipx86 | |
These can be combined. |
chipx86 | |
This looks redundant. |
chipx86 | |
Is this intentional? |
chipx86 | |
As an optimization, you can have a regex for {[^}} and use re.sub with a callback function. In this case, … |
chipx86 | |
Probably don't want to wrap the "uri-template-name". |
chipx86 | |
No blank line. Some docs would be nice. |
chipx86 | |
resource's |
chipx86 | |
The file is synch.py, but we use Sync. I'm thinking we should call the file sync.py. |
chipx86 | |
Should be using super(ClassName, self).__setattr__. |
chipx86 | |
Can you just use self.__getattribute__? |
chipx86 | |
ReviewBot? :) Just food for thought: It might be nice to make the user agent configurable. Some app out there … |
chipx86 | |
Params should be aligned after the (. |
chipx86 | |
Docs for this? |
chipx86 | |
We don't seem to do anything with this. |
chipx86 | |
What are we catching? |
chipx86 | |
Maybe define these once in this class as a constant. |
chipx86 | |
Minor thing, but if you flip these two checks (check for 'href' in dict_keys first), it'll be a bit faster. … |
chipx86 | |
No blank line here. |
chipx86 | |
resource's |
chipx86 | |
resource's |
chipx86 | |
I feel like this would be nicer with "self, name, value" on the second line, indented. Same with the one … |
chipx86 | |
Is this a "just in case" exception, or are there any things specifically we're excepting? If the former, we should … |
chipx86 | |
Is this always correct? Shouldn't this be based on the number of items? |
chipx86 | |
Instead of this and the default value above, just do: item_content_type = info.get('Item-Content-Type', None) |
chipx86 | |
This class and the ones below should, I think, be moved out into a separate file. They're useful outside of … |
chipx86 | |
Should probably be defined once globally. No need to define it every time we create a resource. |
chipx86 | |
Generally, I prefer a list comprehension when possible. dict([ (key.replace('-', '_'), value) for key, value in args.iteritems() ]) |
chipx86 | |
Can we do that now? Has the API changed, or can we just do a try/except with the imports at … |
chipx86 | |
Tiny thing, but maybe put the meth(...) on the second line. |
chipx86 | |
Is there any reason to set this anyway? counts-only is opt-in, so if we don't want it, I don't think … |
chipx86 | |
Any reason to set token to None? We never do anything with it. I'd think we could set it inline … |
chipx86 | |
Should be defined once outside the class. Less work to do each time we instantiate one of these. |
chipx86 |
Change Summary:
Made the changes from Christian's review, and "finished" the sync transport implementation. Updated the description to reflect the current status. A lot more documentation was added, hopefully making things more clear.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 2 (+1000) |
-
-
-
-
-
-
-
-
-
-
-
rbtools/api/transport/sync.py (Diff revision 2) Maybe define these once in this class as a constant.
-
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.
-
-
-
-
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.
-
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.
-
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...
-
rbtools/api/transport/sync.py (Diff revision 2) Is this always correct? Shouldn't this be based on the number of items?
-
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)
-
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.)
Change Summary:
Updated from Christian's review. Added handling for when the server cannot be reached (An API specific exception is now raised).
Diff: |
Revision 3 (+1008) |
---|
Change Summary:
Updated based on Christian's review, fixed support for file uploads in the HttpRequest class, added support for passing url query string arguments to the HttpRequest.
Diff: |
Revision 4 (+1051) |
---|
Change Summary:
Fixed a bug which was causing item resources returned from a list resources payload to be using the generic resource classes, instead of their resource specific base classes.
Diff: |
Revision 5 (+1055) |
---|
Change Summary:
Removed WIP from description. Fixed the HttpRequest class to properly support files with unicode. Removed the TODO comment about checking payload for list (The way resources are constructed by the factory ensures it is a list).
Summary: |
|
||||
---|---|---|---|---|---|
Diff: |
Revision 6 (+1059) |
Change Summary:
Added an acknowledgement to the description.
Description: |
|
---|
-
Just about done. A few small nits but they're minor.
-
rbtools/api/factory.py (Diff revision 6) Should probably be defined once globally. No need to define it every time we create a resource.
-
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() ])
-
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?
-
rbtools/api/resource.py (Diff revision 6) Tiny thing, but maybe put the meth(...) on the second line.
-
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.
-
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__.
-
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.