Add Accounts resource by oritheorca · Pull Request #6 · button/button-client-python

oritheorca · 2016-09-30T20:16:23Z

Accounts resource methods:

accounts.all
accounts.transactions

griffinmyers

A few minor thoughts. Let's discuss and then get this moved out!

griffinmyers · 2016-10-10T21:16:56Z

README.rst

+
+    response = client.accounts.transactions('acc-123',
+        start='2016-07-15T00:00:00.000Z',
+        end='2016-09-30T00:00:00.000Z'


nit: trailing commas

griffinmyers · 2016-10-10T21:34:32Z

README.rst

+    print(response)
+    # <class pybutton.Response [100 elements]>
+
+    cursor = response.next()


#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?

griffinmyers · 2016-10-10T21:36:30Z

pybutton/request.py

+    HTTPError,
+    request,
+    request_url,
+    urlparse,


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.

griffinmyers · 2016-10-10T21:37:17Z

pybutton/resources/accounts.py

+
+
+class Accounts(Resource):
+    '''Manages interacting with Button Orders with the Button API


Docstring needs update

griffinmyers · 2016-10-10T21:37:53Z

pybutton/resources/accounts.py

+
+        return self.api_get('/v1/affiliation/accounts')
+
+    def transactions(self, account_id, cursor=None, start=None, end=None):


May be worth spelling out in the docs what arguments are required and what are optional.

griffinmyers · 2016-10-10T21:45:40Z

pybutton/response.py

+
+    def _format_cursor(self, cursor_url):
+        if cursor_url:
+            query_string = urlparse(cursor_url)[4]


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

griffinmyers · 2016-10-10T21:47:23Z

pybutton/response.py

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


nit: factor out <class pybutton.Response

griffinmyers · 2016-10-10T21:51:19Z

test/response_test.py

+        response = Response({}, response_data)
+        self.assertEqual(response.data(), response_data)
+
+    def test_next(self):


Some tests with botched next/prev commands would be 👍

griffinmyers · 2016-10-10T21:53:49Z

README.rst

    print(response)
    # <class pybutton.Response >

+Accounts


A section in the readme talking about the nature of our pybutton.Response class is worthwhile at this point, I think!

oritheorca · 2016-10-12T18:46:52Z

FYI, this is ready for another look!

griffinmyers

code lgtm! just another round of cosmetic suggestions. Let me know when addressed and we'll get 2.0.0 out da door.

griffinmyers · 2016-10-12T21:46:52Z

README.rst

+Methods
+~~~~~~~
+
+Data


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.

griffinmyers · 2016-10-12T21:47:40Z

README.rst

-        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


Unclear what you mean by "format". The interface is always the same, right?

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!

griffinmyers · 2016-10-12T21:48:20Z

README.rst

+NextCursor
+''''''''''
+
+For any paged resource, ``nextCursor()`` will return a cursor to


I totally borked my recommendation for nextCursor, we should snake_case :-/

griffinmyers · 2016-10-12T21:49:00Z

README.rst

+''''''''''
+
+For any paged resource, ``nextCursor()`` will return a cursor to
+supply for the next page of results.


If next_cursor returns None, there are no more results.

griffinmyers · 2016-10-12T21:50:10Z

README.rst

+    cursor = response.nextCursor()
+
+    # loop through and print all transactions
+    while cursor:


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.

griffinmyers · 2016-10-12T21:57:27Z

pybutton/request.py

+    return urlunsplit((scheme, netloc, path, query, ''))

-__all__ = [Request, urlopen, HTTPError, request, request_url]
+def query_dict(url):


nit: docstring

griffinmyers · 2016-10-12T21:58:27Z

pybutton/response.py

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


update method names

griffinmyers · 2016-10-12T21:58:50Z

pybutton/response.py

    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


response_data

griffinmyers · 2016-10-12T21:59:41Z

test/resources/accounts_test.py

+    'hostname': 'api.usebutton.com',
+    'secure': True,
+    'port': 443,
+    'timeout': None


nit: trailing comma

griffinmyers · 2016-10-12T22:02:26Z

CHANGELOG.md

@@ -1,3 +1,5 @@
+2.0.0 October 10, 2016


I think we also need to include here a point about the breaking changes to pybutton.Response

griffinmyers

lgtm!

griffinmyers · 2016-10-13T17:56:56Z

gckwan added 4 commits September 27, 2016 16:03

Add config options to python client

aa8e608

Update README

d43dedf

Address comments

d2fda7a

Accounts resource

7f0052f

oritheorca changed the base branch from master to grace/options September 30, 2016 20:34

gckwan added 2 commits October 10, 2016 10:28

Merge config changes

1a98942

Fix lint issues

794015e

oritheorca changed the base branch from grace/options to master October 10, 2016 19:43

gckwan added 2 commits October 10, 2016 15:51

Tweaks

4da26be

Update changelog

0b3a183

oritheorca assigned griffinmyers Oct 10, 2016

griffinmyers reviewed Oct 10, 2016

View reviewed changes

Address comments

6c3cbe5

griffinmyers suggested changes Oct 12, 2016

View reviewed changes

gckwan added 2 commits October 13, 2016 09:49

Style nits

daec3a4

Better docstring

0ef6f39

griffinmyers approved these changes Oct 13, 2016

View reviewed changes

oritheorca merged commit 82a7485 into master Oct 13, 2016

oritheorca deleted the grace/accounts branch October 13, 2016 21:23



		class Accounts(Resource):
		'''Manages interacting with Button Orders with the Button API


		return self.api_get('/v1/affiliation/accounts')

		def transactions(self, account_id, cursor=None, start=None, end=None):

+              Methods
+              ~~~~~~~
+              Data

Conversation

oritheorca commented Sep 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

griffinmyers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

griffinmyers Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oritheorca commented Oct 12, 2016

Uh oh!

griffinmyers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

griffinmyers left a comment

Choose a reason for hiding this comment

Uh oh!

griffinmyers commented Oct 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

oritheorca commented Sep 30, 2016 •

edited

Loading

griffinmyers Oct 10, 2016 •

edited

Loading