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

Added Aiohttp based client, added FutureResponse to lazily load response #61

Merged
merged 18 commits into from
Nov 5, 2017
Merged

Added Aiohttp based client, added FutureResponse to lazily load response #61

merged 18 commits into from
Nov 5, 2017

Conversation

dancollins34
Copy link

@dancollins34 dancollins34 commented Nov 5, 2017

This adds an aiohttp asynchronous http client which uses futures. It also adds a FutureResponse object which will not automatically load responses on initialization, only on access.

Of note- The description on the wiki that you can monkey patch with requests using eventlet is incorrect. While you can do this with no issue, the fact that the default Response accesses all fields upon initialization would require waits every time the Response object was created and defeat the purpose of this.

Also to note- this only works for python>=3.5 due to the use of async await syntax. There are checks in place in the code to throw a runtime error in earlier versions

@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage decreased (-0.2%) to 99.826% when pulling 7d92cd0 on dancollins34:master into d409c27 on joowani:dev.

@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage decreased (-0.2%) to 99.826% when pulling 758f6a9 on dancollins34:master into d409c27 on joowani:dev.

Removed the restriction on FutureResponse, given that it will work with any future, not just an asyncio one.  Removed the error which gets thrown in async client init, since earlier imports will fail due to the async keyword, tested the failure on version number in http_clients/__init__.py
@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage decreased (-0.09%) to 99.913% when pulling 8cae98c on dancollins34:master into d409c27 on joowani:dev.

@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage decreased (-0.04%) to 99.956% when pulling 7d362f6 on dancollins34:master into d409c27 on joowani:dev.

@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling c056dc3 on dancollins34:master into d409c27 on joowani:dev.

@joowani
Copy link
Contributor

joowani commented Nov 5, 2017

Hi @dancollins34,

Thank you for the PR. I've been meaning to look into asyncio myself but never had the time... so this is awesome! Just a few requests:

  • Please squash all the commits into one
  • Use present tense for the commit message for consistency (e.g. "Add documentation" preferred over "Added documentation")
  • Make sure all the names are consistent (e.g. "return_future" over "returnFuture").
  • Instead of using "aio" can we be more explicit and go with "asyncio"? Are there name conflict issues that are forcing you to use "aio"?

I appreciate your contribution!

Cheers,
Joohwan

@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling a95effa on dancollins34:master into d409c27 on joowani:dev.

commit a95effa
Author: dancollins34 <dancollins34@gmail.com>
Date:   Sat Nov 4 22:45:13 2017 -0400

    Added file rename to asyncio.py, changed returnFuture to return_future

commit c056dc3
Author: dancollins34 <dancollins34@gmail.com>
Date:   Sat Nov 4 21:40:43 2017 -0400

    Added test_update_user to increase coverage to 100% for aiohttp client

commit 7d362f6
Author: dancollins34 <dancollins34@gmail.com>
Date:   Sat Nov 4 21:30:56 2017 -0400

    Fixed the last coverage error related to FutureResponse

commit 8cae98c
Author: dancollins34 <dancollins34@gmail.com>
Date:   Sat Nov 4 21:16:09 2017 -0400

    Made changes to fix travis and coverage

    Removed the restriction on FutureResponse, given that it will work with any future, not just an asyncio one.  Removed the error which gets thrown in async client init, since earlier imports will fail due to the async keyword, tested the failure on version number in http_clients/__init__.py

commit 9e32673
Author: dancollins34 <dancollins34@gmail.com>
Date:   Sat Nov 4 21:07:38 2017 -0400

    Revert "Added tests to bring to full coverage of version exceptions"

    This reverts commit 758f6a9.

commit 758f6a9
Author: dancollins34 <dancollins34@gmail.com>
Date:   Sat Nov 4 21:03:14 2017 -0400

    Added tests to bring to full coverage of version exceptions

commit 7d92cd0
Author: dancollins34 <dancollins34@gmail.com>
Date:   Sat Nov 4 20:48:16 2017 -0400

    Moved utils to tests.utils, replaced references to be correct

commit 5de0ab7
Author: dancollins34 <dancollins34@gmail.com>
Date:   Sat Nov 4 20:44:48 2017 -0400

    Fixed an error in setup.py

commit 60cb3e0
Author: dancollins34 <dancollins34@gmail.com>
Date:   Sat Nov 4 20:42:26 2017 -0400

    Fixed bash math

commit 1dff440
Author: dancollins34 <dancollins34@gmail.com>
Date:   Sat Nov 4 20:27:15 2017 -0400

    Incorrect direction for versioned import

commit 51e107b
Author: dancollins34 <dancollins34@gmail.com>
Date:   Sat Nov 4 20:22:11 2017 -0400

    Fixed import statement to only import asyncio client when applicable

commit f5f27c2
Author: dancollins34 <dancollins34@gmail.com>
Date:   Sat Nov 4 20:18:58 2017 -0400

    Forgot to close travis bash statement if blocks

commit 6c2b2cc
Author: dancollins34 <dancollins34@gmail.com>
Date:   Sat Nov 4 20:11:56 2017 -0400

    Attempted to fix multiple test possibilities, added imports to setup.py

commit 3ae18be
Author: dancollins34 <dancollins34@gmail.com>
Date:   Sat Nov 4 19:42:41 2017 -0400

    Added aiohttp based client, added FutureResponse to lazily load response attributes
@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 2cbe139 on dancollins34:master into d409c27 on joowani:dev.

@dancollins34
Copy link
Author

Squashed commits. Im unsure how to change the commit messages, but I can do so in the future. All the names are now consistent and name of file changed from aio.py to asyncio.py

- Add tests for aiohttp based client
- Restrict import in http_clients.__init__.py to applicable python versions
- Add FutureResponse to lazily load attributes
- Move testing functions to general, py35 and py34_minus submodules
@dancollins34
Copy link
Author

Fixed the commit message

@coveralls
Copy link

coveralls commented Nov 5, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 6602725 on dancollins34:master into d409c27 on joowani:dev.

@joowani joowani merged commit 42930c9 into arangodb:dev Nov 5, 2017
@joowani
Copy link
Contributor

joowani commented Nov 5, 2017

Hi @dancollins34,

Thanks again for the PR. I've merged it to dev for now. As I am a novice when it comes to asyncio, it will take some time before I can vet the changes and push it to master branch. Meanwhile, I will update the documentation on eventlet monkey patching (thank you for pointing that out as well).

Best,
Joohwan

@dancollins34
Copy link
Author

Realized that, although this works fine, it still will provide not performance benefits because responses are immediately accessed by the collection functions. I'll work on a refactor to remove this immediate access tonight.

@joowani
Copy link
Contributor

joowani commented Nov 6, 2017

Hi @dancollins34,

I'm currently in the process of refactoring some of the changes you've made + adding other improvements. Also it seems the commits were not squashed properly so I'm fixing that as well. This will likely result in a rebase so if you don't mind, it might be best if you wait a day or two so I can push the change to the dev branch first (at which point you will probably have to reset your local repository. Otherwise you will have to deal with git conflicts etc.

Thanks,
Joohwan

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.

3 participants