search

closeio / closeio-api

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

Clean up the code, add docstrings, and get rid of the poorly implemented async client #78

Closed wojcikstefan closed 7 years ago

wojcikstefan commented 7 years ago

Right now the async client makes the code more complex and doesn't really add much value. I doubt it's used in the wild and if it is, then the usage is probably quite confusing and hacky. I think we might as well get rid of it.

Examples of current confusing behavor:

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

TODO:

wojcikstefan commented 7 years ago

Bonus point against async code - due to monkey patching upon import, grequests can cause warnings/bugs that might be very hard to find. See:

thomasst commented 7 years ago

Looks good (but didn't test).

wojcikstefan commented 7 years ago

@closeio/engineering I believe that once we add unit tests to this PR, we can release it as v1.0. Agreed?

philfreo commented 7 years ago

👍

thomasst commented 7 years ago

Sounds good. So what's happening to async? Are there any plans for it?

wojcikstefan commented 7 years ago

Given the arguments I laid out in https://github.com/closeio/closeio-api/pull/78#issuecomment-275871159 and in the description for #79, I think it might make sense to drop it altogether. This way we maintain a simple & readable helper library w/o any quirks while the users can choose to run multiple concurrent api.get/post/put/delete requests via threading, multiprocessing, concurrent.futures, gevent, etc. depending on their specific use case and environment.

philfreo commented 7 years ago

@wojcikstefan Let's move forward with merging this? Can you handle conflicts and publish?

wojcikstefan commented 7 years ago

@thomasst @jkemp101 @philfreo This should be ready to go, please review. If you give the go ahead, I'll merge the changes, bump up the version, create a new release, and deploy to PyPI.