Add Accounts resource #6
Conversation
There was a problem hiding this comment.
A few minor thoughts. Let's discuss and then get this moved out!
| response = client.accounts.transactions('acc-123', | ||
| start='2016-07-15T00:00:00.000Z', | ||
| end='2016-09-30T00:00:00.000Z' |
README.rst
Outdated
| print(response) | ||
| # <class pybutton.Response [100 elements]> | ||
| cursor = response.next() |
There was a problem hiding this comment.
#next is somewhat of a special method name in python 2.x. It's a part of the iteration protocol that we should try to avoid colliding with. #nextCursor?
pybutton/request.py
Outdated
| HTTPError, | ||
| request, | ||
| request_url, | ||
| urlparse, |
There was a problem hiding this comment.
instead of exporting these functions, you could consider exporting a handy function that encapsulates the behavior you want to expose, which is converting a full URL into a query dict, I imagine.
pybutton/resources/accounts.py
Outdated
|
|
||
|
|
||
| class Accounts(Resource): | ||
| '''Manages interacting with Button Orders with the Button API |
There was a problem hiding this comment.
Docstring needs update
|
|
||
| return self.api_get('/v1/affiliation/accounts') | ||
|
|
||
| def transactions(self, account_id, cursor=None, start=None, end=None): |
There was a problem hiding this comment.
May be worth spelling out in the docs what arguments are required and what are optional.
pybutton/response.py
Outdated
|
|
||
| def _format_cursor(self, cursor_url): | ||
| if cursor_url: | ||
| query_string = urlparse(cursor_url)[4] |
There was a problem hiding this comment.
Can we add more sanity checks around these list look-ups? I'd like to make sure the API can start returning botched URLs and this client quietly falls back to retuning None
pybutton/response.py
Outdated
| if isinstance(self.response_data, dict): | ||
| for k, v in self.response_data.items(): | ||
| values = values + ['{0}: {1}'.format(k, v)] | ||
| return '<class pybutton.Response {0}>'.format(', '.join(values)) |
There was a problem hiding this comment.
nit: factor out <class pybutton.Response
test/response_test.py
Outdated
| response = Response({}, response_data) | ||
| self.assertEqual(response.data(), response_data) | ||
|
|
||
| def test_next(self): |
There was a problem hiding this comment.
Some tests with botched next/prev commands would be 👍
| print(response) | ||
| # <class pybutton.Response > | ||
| Accounts |
There was a problem hiding this comment.
A section in the readme talking about the nature of our pybutton.Response class is worthwhile at this point, I think!
|
FYI, this is ready for another look! |
There was a problem hiding this comment.
code lgtm! just another round of cosmetic suggestions. Let me know when addressed and we'll get 2.0.0 out da door.
README.rst
Outdated
| Methods | ||
| ~~~~~~~ | ||
|
|
||
| Data |
There was a problem hiding this comment.
because these are methods, let's leave them lowercased. A little weird, but the sections above are conceptually "Create", not meaning to refer to the method name.
README.rst
Outdated
| start='2016-07-15T00:00:00.000Z', | ||
| end='2016-09-30T00:00:00.000Z' | ||
| ) | ||
| The format of the ``pybutton.Response`` class varies depending on whether it contains |
There was a problem hiding this comment.
Unclear what you mean by "format". The interface is always the same, right?
There was a problem hiding this comment.
I meant the printed output from __repr__, but you're right, this is unclear. It doesn't add much anyway, so I'll remove this part!
README.rst
Outdated
| NextCursor | ||
| '& A3DB #39;'''''''' | ||
|
|
||
| For any paged resource, ``nextCursor()`` will return a cursor to |
There was a problem hiding this comment.
I totally borked my recommendation for nextCursor, we should snake_case :-/
README.rst
Outdated
| '''''''''' | ||
|
|
||
| For any paged resource, ``nextCursor()`` will return a cursor to | ||
| supply for the next page of results. |
There was a problem hiding this comment.
If
next_cursorreturnsNone, there are no more results.
| cursor = response.nextCursor() | ||
| # loop through and print all transactions | ||
| while cursor: |
There was a problem hiding this comment.
This is a cool method we can add later to the Response class... all_pages or something, where it returns a generator that yields all pages.
| return urlunsplit((scheme, netloc, path, query, '')) | ||
|
|
||
| __all__ = [Request, urlopen, HTTPError, request, request_url] | ||
| def query_dict(url): |
pybutton/response.py
Outdated
| It exposes all keys in the underlying response as attributes on the | ||
| instance. | ||
| It exposes the response data via the `data` method and cursors for | ||
| pagination via the `next`/`prev` methods. |
pybutton/response.py
Outdated
| Args: | ||
| attrs (dict): The underlying response value from an API call | ||
| meta (dict): The metadata from an API call | ||
| data (dict or array<dict>): The response elements from an API call |
test/resources/accounts_test.py
Outdated
| 'hostname': 'api.usebutton.com', | ||
| 'secure': True, | ||
| 'port': 443, | ||
| 'timeout': None |
CHANGELOG.md
Outdated
| @@ -1,3 +1,5 @@ | |||
| 2.0.0 October 10, 2016 | |||
There was a problem hiding this comment.
I think we also need to include here a point about the breaking changes to pybutton.Response
|
|
Accounts resource methods:
accounts.allaccounts.transactions