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 asyncio support #257

Open
diogommartins opened this issue Aug 19, 2018 · 6 comments
Open

Add asyncio support #257

diogommartins opened this issue Aug 19, 2018 · 6 comments

Comments

@diogommartins
Copy link
Contributor

Currently marathon-python depends has a MarathonClient that depends on requests. Would you accept an AsyncMarathonClient version that uses aiohttp ? I would gladly submit a PR

@solarkennedy
Copy link
Contributor

Gosh. I'm not sure. @EvanKrall what do you think?

@EvanKrall
Copy link
Contributor

EvanKrall commented Aug 20, 2018

I'd be in favor, and would probably work on the PR to Yelp/paasta to use it.

Fortunately, it doesn't look like any of the model objects are doing any lazy fetching of data, so the model objects' API should be able to stay the same, and it looks like just a separate AsyncMarathonClient object would be sufficient.

@EvanKrall
Copy link
Contributor

I'm interested to see how you test the async version. Ideally we'd have mostly the same set of tests for both async and non-async. In general, I care less about DRY in test code than non-test code, so if you end up having to copy-paste-sed the tests, that's fine, but ideally we'd enforce test parity between the two versions somehow.

@diogommartins
Copy link
Contributor Author

Great!! I'll keep in mind the test suite during the development of AsyncMarathonClient.
FYI: At B2W we value testing and we use marathon-python on a production environment as a dependency of asgard-api, a flask API that we are currently thinking about moving to asyncio.

@diogommartins
Copy link
Contributor Author

btw, the current marathon-python CI build runs only against py2.7 and implementing
AsyncMarathonClient would break compatibility with py2.7. Thats ok ? If so, its ok to support only py>=3.6 ?

@EvanKrall
Copy link
Contributor

It looks like we're testing against py2.7 and py3.3: travis calls make test, which calls tox -e test-py27 and tox -e test-py33.

I'd be okay with bumping the python3 version requirement to 3.6; I don't think I can speak with the same authority for python 2.7 - I don't know how many python 2.7 users use this library. Would it be onerous to put the async client in a different module so that python 2.7 users can still use the requests-based client?

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

No branches or pull requests

3 participants