Add config options to python client by oritheorca · Pull Request #5 · button/button-client-python

oritheorca · 2016-09-27T20:16:55Z

Description

Add hostname, secure, port, timeout options

Test Plan

Updated unit tests to include configs
Tested against api.usebutton, staging, SimpleHTTPServer

griffinmyers

This looks fantastic, @gckwan! I've requested a few cosmetic changes and the move to urlparse... after which I think we'll be ready to ship!

Because the API is backwards compatible, we'll also want to bump the package minor version and add a note to the CHANGELOG

griffinmyers · 2016-09-27T20:39:20Z

+
+.. code:: python
+
+  client = Client("sk-XXX", {


Can you include from pybutton import Client? I'm trying to keep all code snippets atomically copy+pasteable

griffinmyers · 2016-09-27T20:40:41Z

+    'hostname': 'api.testsite.com',
+    'port': 80,
+    'secure': False,
+    'timeout': 5


maybe 'timeout': 5 # seconds just to make sure people copy+pasting know the units... It's fairly common that timeouts are expressed in ms.

griffinmyers · 2016-09-27T20:41:33Z

            https://app.usebutton.com/settings/organization.

+        config (dict): Configuration options for the client. Options include:
+            timeout: The time in ms for network requests to abort. Defaults to None.


nit: match ordering of README

griffinmyers · 2016-09-27T20:44:35Z

    '''

-    def __init__(self, api_key):
+    def __init__(self, api_key, config={}):


Python function (and bound method) definitions hold the memory for any default parameters. This means if you somehow mutate a default parameter argument (and believe me it happens) you'll be mutating the default for all following invocations. The "pythonic" move here is to default to None:

def __init__(self, api_key, config=None): if config is None: config = {}

weird! good to know, thanks.

griffinmyers · 2016-09-27T21:48:25Z

+
+        self.orders = Orders(api_key, config)
+
+    def _config_with_defaults(self, config):


Because this method doesn't actually need to be bound to self, we can just have this as a function in the file.

griffinmyers · 2016-09-27T22:03:33Z

+
+    def _request_url(self, path):
+        protocol = 'https://' if self.config['secure'] else 'http://'
+        return '{0}{1}:{2}{3}'.format(protocol, self.config['hostname'], self.config['port'], path)


I think at this point it makes sense to get some help from the python stdlib:

import urlparse # 'http://api.usebutton.com/12/12' urlparse.urlunsplit(('http', 'api.usebutton.com', '/12/12', '', '')) # also 'http://api.usebutton.com/12/12' urlparse.urlunsplit(('http', 'api.usebutton.com', '12/12', '', ''))

griffinmyers · 2016-09-27T22:05:34Z

-    def __init__(self, api_key):
+    def __init__(self, api_key, config):
        self.api_key = api_key
+        self.config = config


even though you're going to end up repeating it a lot, I think a docstring here explaining config makes sense (especially given all the keys are required at this point)

Totally. Added config to the docstring above.

griffinmyers · 2016-09-27T22:14:22Z

    # {'status': open, 'btn_ref': None, 'line_items': [], ...}

+Configuration
+---------


nit: extra -'s

oritheorca · 2016-10-03T20:13:09Z

@griffinmyers Ping! No rush, but ready for another look whenever you have free time.

griffinmyers · 2016-10-03T20:13:49Z

👋 @gckwan cool! will take a look today!

griffinmyers

A couple style nits, specifically please run the linter flake8. I saw a couple violations. You can get your own copy with pip install flake8.

Upshot: I tested locally and everything looks great functionality wise!

griffinmyers · 2016-10-03T21:23:19Z

@@ -1,2 +1,5 @@
 1.0.2 August 11, 2016
  - Initial Release
+1.1.0 September 28, 2016


nit: changelog should be sorted reverse chronological order

griffinmyers · 2016-10-03T21:23:45Z

-            'hostname': config.get('hostname', 'api.usebutton.com'),
-            'port': config.get('port', defaultPort)
-        }
+def _config_with_defaults(config):


nit: because this isn't a private method to anything we can drop the _ prefix on the function name.

griffinmyers · 2016-10-03T21:35:40Z

+        'secure': secure,
+        'timeout': config.get('timeout'),
+        'hostname': config.get('hostname', 'api.usebutton.com'),
+        'port': config.get('port', defaultPort)


nit: as weird as it may seem, the python community seems to prefer trailing commas for multi-line dict/list elements. config.get('port', defaultPort),

I actually like trailing commas, myself! No need to add/remove commas when reordering values/adding new lines.

The airbnb styleguide is pro-trailing commas too: https://github.com/airbnb/javascript#commas--dangling

still need to add trailing commas! I've seen some in README / tests we should make sure get them as well.

Derp, got too focused on the flake8 output. Adding them now.

griffinmyers · 2016-10-03T21:36:59Z

-        protocol = 'https://' if self.config['secure'] else 'http://'
-        return '{0}{1}:{2}{3}'.format(protocol, self.config['hostname'], self.config['port'], path)
+        protocol = 'https' if self.config['secure'] else 'http'
+        return urlparse.urlunsplit((protocol, '{0}:{1}'.format(self.config['hostname'], self.config['port']), path, '', ''))


nit: I try go give best effort to keeping under 80 columns. This could easily be split up with 1 argument per line or maybe by allocating the hostname/port string ahead of the #urlunsplit call

oritheorca · 2016-10-04T17:42:53Z

flake8 should pass now :)

griffinmyers · 2016-10-04T18:51:29Z

            raise ButtonClientError('Invalid response: {0}'.format(response))

-    __all__ = [Request, urlopen, HTTPError, request]
+    def request_url(secure, hostname, port, path):


because this is the exact same implementation as the one on line 117, we can simply import each urlunsplit in the if/else and the define request_url and __all__ at the bottom of the file, once. Also, would you mind adding a Args/Returns to the docstring on this because it's used external to this file?

griffinmyers

🎉 LGTM!

griffinmyers · 2016-10-04T21:23:55Z

Looks awesome! When you merge, please select the Squash and merge option. Then we can publish!

gckwan added 2 commits September 27, 2016 16:03

Add config options to python client

aa8e608

Update README

d43dedf

oritheorca force-pushed the grace/options branch from 83a7fcf to d43dedf Compare September 27, 2016 20:27

oritheorca assigned griffinmyers Sep 27, 2016

griffinmyers suggested changes Sep 27, 2016

View reviewed changes

Address comments

d2fda7a

oritheorca force-pushed the grace/options branch from 88f267e to d950bfc Compare September 30, 2016 20:28

move request_url into requests

05084cf

oritheorca force-pushed the grace/options branch from d950bfc to 05084cf Compare September 30, 2016 20:34

oritheorca mentioned this pull request Sep 30, 2016

Add Accounts resource #6

Merged

griffinmyers suggested changes Oct 3, 2016

View reviewed changes

Fix lint errors

afd9d75

griffinmyers reviewed Oct 4, 2016

View reviewed changes

gckwan added 4 commits October 4, 2016 15:16

Single definition for request_url

8c59aeb

Trailing commas, update changelog date

f657497

request_url docstring

5421503

one more comma

26c64f6

griffinmyers approved these changes Oct 4, 2016

View reviewed changes

oritheorca merged commit f86c72a into master Oct 10, 2016

griffinmyers deleted the grace/options branch January 5, 2017 23:32


		self.orders = Orders(api_key, config)

		def _config_with_defaults(self, config):

Conversation

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

Uh oh!

Description

Test Plan

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!

oritheorca commented Oct 3, 2016

Uh oh!

griffinmyers commented Oct 3, 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!

oritheorca commented Oct 4, 2016

Uh oh!

griffinmyers Oct 4, 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!

griffinmyers left a comment

Choose a reason for hiding this comment

Uh oh!

griffinmyers commented Oct 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

oritheorca commented Sep 27, 2016 •

edited

Loading

griffinmyers Oct 4, 2016 •

edited

Loading