search

closeio / closeio-api

Python API Client for Close
http://developer.close.com/
MIT License
65 stars 47 forks source link

Make the async version of the client easier to use #79

Closed wojcikstefan closed 7 years ago

wojcikstefan commented 7 years ago

This is an alternative to #78 fixing some of the async client's flaws instead of removing it altogether.

Example usage:

In [1]: from closeio_api import Client

In [2]: api = Client('api_key', development=True, async=True)

In [3]: results = api.map([
   ...:   api.post('lead', {'name': 'New Lead 1'}),
   ...:   api.post('lead', {'name': 'New Lead 2'}),
   ...:   api.post('lead', {'name': 'New Lead 3'}),
   ...:   api.post('lead', {'name': 'New Lead 4'})
   ...: ])

In [4]: len(results)
Out[4]: 4

In [5]: results[0]['id']
Out[5]: u'lead_RpzCIwspNlTUowYUvIUaNE1tp3ncVFk8x3Uw4sv96FO'

I'm still conflicted whether such a deep integration of concurrency is good for this client library and its users or not. On the one hand, it makes concurrent requests easier to construct and send, and it handles API errors and retry logic properly (-ish). On the other hand, it makes the code more complex (and thus harder to read/understand), it forces a particular implementation of concurrency (green threads & gevent), and there are still some quirks with it (e.g. using the "debug" flag differs unintuitively between the sync and async client).

@closeio/engineering what do you think?

This PR introduces breaking changes and should be published along with a major version bump.

thomasst commented 7 years ago

It seems weird that api.post() wouldn't actually post a request with an async client. Is there a less confusing syntax we could use?

wojcikstefan commented 7 years ago

Hm, I actually thought that it's more intuitive than the alternative where an async client sends requests synchronously upon api.get/post/put/delete. Might be just my personal preference though...

An alternative could be to have api.prepare_get/prepare_post/etc. which would internally use _prepare_request.

@thomasst did you also take a look at #78 ?

wojcikstefan commented 7 years ago

Closing this PR as it seems we're all in consensus that such a specific async implementation within our library is not necessary. Will follow the path that PR #78 lays out instead.