Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add proxy support [#16] #17

Merged
merged 4 commits into from
Aug 22, 2017
Merged

Add proxy support [#16] #17

merged 4 commits into from
Aug 22, 2017

Conversation

dnl-blkv
Copy link
Contributor

Fixes #16

@dnl-blkv
Copy link
Contributor Author

@OGKevin deserialisation is fixed! 👍

@OGKevin
Copy link
Contributor

OGKevin commented Aug 22, 2017

@dnl-blkv My tests are failing while creating new context.

File "/Users/khellemun/GitHub/bunq/sdk_python/bunq/sdk/client.py", line 105, in _request
    proxies={self._FIELD_PROXY_HTTPS: self._api_context.proxy_url}
AttributeError: 'ApiContext' object has no attribute 'proxy_url'

api_context = ApiContext(ApiEnvironmentType.SANDBOX, cls._API_KEY,

What could be causing this ?

@OGKevin
Copy link
Contributor

OGKevin commented Aug 22, 2017

Hmm weird just did a clean checkout again merged dev and tests are passing 🤔

@@ -57,12 +58,13 @@ class ApiContext(object):
_PATH_API_CONTEXT_DEFAULT = 'bunq.conf'

def __init__(self, environment_type, api_key, device_description,
permitted_ips=None):
permitted_ips=None, proxy_url=None):
Copy link
Contributor

@OGKevin OGKevin Aug 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function has a lot of arguments, why not reformat it to:

def __init__(self, environment_type, api_key,
                   device_description,  permitted_ips=None, proxy_url=None):

(nicely aligned of course 😉 )
3 args on a row or:

    def __init__(
            self,
            environment_type,
            api_key,
            device_description,
            permitted_ips=None,
            proxy_url=None):

1 arg per line ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Not PEP-8
  2. Takes too much space

This one is OK, we do it everywhere :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

Copy link
Contributor

@OGKevin OGKevin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests passing 👏 looks good 🏅

@andrederoos

@OGKevin OGKevin merged commit e60081b into develop Aug 22, 2017
@OGKevin OGKevin deleted the 16_proxy branch August 22, 2017 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants