Support python requests keyword arguments when making remote calls #1

banjoh · 2017-01-31T06:01:14Z

Since this library is heavily dependent on python requests for making remote calls, it is necessary to pass down parameters such as timeout and header to requests methods.

This PR is related to simple-salesforce#148

kryptidikettu

I see you have lots of changes here and I partly disagree with your style in RequestMixin, see the comments in the diff for more info :).

kryptidikettu · 2017-01-31T11:16:49Z

simple_salesforce/api.py

@@ -37,8 +38,60 @@ def _warn_request_deprecation():
    )


+class RequestMixin(object):
+    def __init__(self, session=None):


MixIn-objects generally do not initialize themselves. See examples here: https://github.com/pallets/werkzeug/blob/master/werkzeug/wrappers.py#L1281-L1628
https://github.com/django/django/blob/master/django/contrib/auth/mixins.py

Also this blog post explains some concepts: https://www.ianlewis.org/en/mixins-and-python

Basic idea behind Mixins (as I understand them), is that you give devs possibilities to add "automatic" features to other classes. In this case, your Mixin would not implement _call_salesforce method, it would implement something like request. The Salesforce-class would then always use self.request()-method but depending on the Mixin inherited, the behaviour would change.

I recommend either renaming this class, or thinking how to implement it in a more "generic" mixin style.

Well, I think this is subject to different interpretations, but I see your point. I'll make some changes to simplify the implementation. Using a mixin may stir the wrong interpretation.

kryptidikettu · 2017-01-31T11:19:15Z

simple_salesforce/api.py

@@ -3,6 +3,7 @@

 # has to be defined prior to login import
 DEFAULT_API_VERSION = '29.0'
+TIMEOUT = 60


I don't think we want to have TIMEOUT as a global property... If the idea is for the user of Salesforce to pass the timeout parameter for requests, timeout should be empty by default.

It could also be configurable when you instantiate the Salesforce class...

kryptidikettu · 2017-01-31T11:22:12Z

simple_salesforce/api.py

+        self.session_id = "sessionId"
+        self.name = ""
+
+    def _call_salesforce(self, method, url, **kwargs):


You defined timeout as a global property, it could be also implemented here as an argument for this function. This would make more sense, since you can also provide default value for it.

kryptidikettu · 2017-01-31T11:25:00Z

simple_salesforce/api.py

        """Describes all available objects
+
+        Keyword Args: Parameters passed down to requests.request method


In this case Keyword Args is actually passed down to requests.Sessions.request-method, not requests.request. The documentation for those functions is (almost) the same, but we want to be accurate in our own documentation :).

Remember the backticks for requests.request. The original documentation also uses * kwargs instead of Keyword Args. Is this change intentional?

kryptidikettu · 2017-01-31T11:34:20Z

simple_salesforce/api.py

@@ -774,22 +807,12 @@ def __init__(self, username, password, security_token, sandbox=False,

 def _exception_handler(result, name=""):


This function has been defined as "private" (by Python naming standards). Why not remove it instead of marking it deprecated?

_single_leading_underscore : weak "internal use" indicator. E.g. from M import * does not import objects whose name starts with an underscore.
-- https://www.python.org/dev/peps/pep-0008/#public-and-internal-interfaces

Use one leading underscore only for non-public methods and instance variables.
-- https://www.python.org/dev/peps/pep-0008/#method-names-and-instance-variables

Any backwards compatibility guarantees apply only to public interfaces. Accordingly, it is important that users be able to clearly distinguish between public and internal interfaces.

Documented interfaces are considered public, unless the documentation explicitly declares them to be provisional or internal interfaces exempt from the usual backwards compatibility guarantees. All undocumented interfaces should be assumed to be internal.
-- https://www.python.org/dev/peps/pep-0008/#descriptive-naming-styles

That was my initial approach but I thought its possible to have someone (mis)use it since its not really private. I would rather go through the deprecation processes.

The project has been forked quite a bit. I would not want to break extensions that exist already.

But as PEP8 describes, it is never a guarantee for undocumented private-functions to stay backward compatible. My personal opinion is to remove this function, but the upstream author has, of course, the ultimate say :).

Edit: You can keep the current implementation, or do my proposed change. It is up to you.

I'll probably remove it anyway. Let's see what the maintainer comments. I'll add a comment about this.

kryptidikettu · 2017-01-31T11:35:56Z

simple_salesforce/login.py

@@ -21,7 +21,7 @@
 def SalesforceLogin(
        username=None, password=None, security_token=None,
        organizationId=None, sandbox=False, sf_version=DEFAULT_API_VERSION,
-        proxies=None, session=None, client_id=None):


Where did the documented proxies Keyword Argument vanish?

It's one of the requests kwargs and hence part of the requests' documentation.

https://github.com/kennethreitz/requests/blob/master/requests/api.py#L16

kryptidikettu · 2017-01-31T11:38:24Z

simple_salesforce/tests/test_api.py

@@ -575,12 +577,14 @@ def setUp(self):
        self.mockresult = Mock()
        self.mockresult.url = 'http://www.example.com/'
        self.mockresult.json.return_value = 'Example Content'
+        self.mock_mixin = RequestMixin()


Why is this called mock_mixin when it is, in fact, the real Mixin-class?

kryptidikettu · 2017-01-31T11:39:54Z

simple_salesforce/tests/test_api.py

+        self.mockresult.status_code = 404
+        resource_name = 'SpecialContacts'
+        with self.assertRaises(SalesforceResourceNotFound) as exception:
+            _exception_handler(result=self.mockresult, name=resource_name)


If you remove the (currently) deprecated _exception_handler, you can also remove this test and the import for _exception_handler :).

I added it there as a regression test. Once it's really deprecated then it can be removed.

kryptidikettu · 2017-01-31T12:52:46Z

simple_salesforce/api.py

@@ -38,7 +37,7 @@ def _warn_request_deprecation():
    )


-class RequestMixin(object):
+class RequestBase(object):
    def __init__(self, session=None):
        """
        Mixin class to encapsulate all remote operations


Missed a docstring here :)

kryptidikettu · 2017-01-31T12:53:37Z

simple_salesforce/api.py

@@ -810,7 +809,7 @@ def _exception_handler(result, name=""):
    warnings.warn('This global exception handler is deprecated. It has been moved'
                  'to the RequestMixin class.', DeprecationWarning)

-    mixin = RequestMixin()
+    mixin = RequestBase()


mixin name appears here as well.

kryptidikettu · 2017-01-31T13:00:26Z

simple_salesforce/api.py

@@ -61,7 +60,7 @@ def _call_salesforce(self, method, url, **kwargs):
        }
        additional_headers = kwargs.pop('headers', dict())
        headers.update(additional_headers or dict())
-        timeout = kwargs.pop('timeout', TIMEOUT)
+        timeout = kwargs.pop('timeout', 60)


I still think you should add timeout as an argument with a default value, make it configurable in the __init__()-function or keep it unchanged in kwargs. This timeout = kwargs.pop looks too verbose and, frankly, ugly here...

kryptidikettu · 2017-01-31T13:00:49Z

simple_salesforce/tests/test_api.py

@@ -577,8 +576,8 @@ def setUp(self):
        self.mockresult = Mock()
        self.mockresult.url = 'http://www.example.com/'
        self.mockresult.json.return_value = 'Example Content'
-        self.mock_mixin = RequestMixin()
-        self.exec_handler = self.mock_mixin._exception_handler
+        self.mixin = RequestBase()


mixin name here as well.

banjoh added bug enhancement labels Jan 31, 2017

banjoh requested a review from kryptidikettu January 31, 2017 06:01

kryptidikettu suggested changes Jan 31, 2017

View reviewed changes

Support python requests keyword arguments when making remote calls

ac113b6

banjoh force-pushed the requests_args branch from 531f990 to ac113b6 Compare January 31, 2017 14:13

banjoh merged commit 6589602 into F-Secure:master Jan 31, 2017

banjoh deleted the requests_args branch January 31, 2017 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support python requests keyword arguments when making remote calls #1

Support python requests keyword arguments when making remote calls #1

banjoh commented Jan 31, 2017 •

edited

Loading

kryptidikettu left a comment

kryptidikettu Jan 31, 2017

banjoh Jan 31, 2017

kryptidikettu Jan 31, 2017

kryptidikettu Jan 31, 2017

kryptidikettu Jan 31, 2017 •

edited

Loading

kryptidikettu Jan 31, 2017

kryptidikettu Jan 31, 2017

kryptidikettu Jan 31, 2017

banjoh Jan 31, 2017

kryptidikettu Jan 31, 2017 •

edited

Loading

banjoh Jan 31, 2017

kryptidikettu Jan 31, 2017

banjoh Jan 31, 2017

kryptidikettu Jan 31, 2017

kryptidikettu Jan 31, 2017

banjoh Jan 31, 2017

kryptidikettu Jan 31, 2017

kryptidikettu Jan 31, 2017

kryptidikettu Jan 31, 2017 •

edited

Loading

kryptidikettu Jan 31, 2017

		"""Describes all available objects

		Keyword Args: Parameters passed down to requests.request method

		@@ -774,22 +807,12 @@ def __init__(self, username, password, security_token, sandbox=False,

		def _exception_handler(result, name=""):

Support python requests keyword arguments when making remote calls #1

Support python requests keyword arguments when making remote calls #1

Conversation

banjoh commented Jan 31, 2017 • edited Loading

kryptidikettu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kryptidikettu Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kryptidikettu Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kryptidikettu Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banjoh commented Jan 31, 2017 •

edited

Loading

kryptidikettu Jan 31, 2017 •

edited

Loading

kryptidikettu Jan 31, 2017 •

edited

Loading

kryptidikettu Jan 31, 2017 •

edited

Loading