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

Adopt aiohttp>=3.0.0 (#519) #536

Merged
merged 1 commit into from
Feb 28, 2018
Merged

Conversation

Gr1N
Copy link
Contributor

@Gr1N Gr1N commented Feb 27, 2018

Dropped support of Python 3.4 and aiohttp < 3.0.0.

@codecov
Copy link

codecov bot commented Feb 27, 2018

Codecov Report

Merging #536 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #536      +/-   ##
==========================================
+ Coverage   90.05%   90.07%   +0.01%     
==========================================
  Files           8        8              
  Lines         523      524       +1     
==========================================
+ Hits          471      472       +1     
  Misses         52       52
Impacted Files Coverage Δ
aiobotocore/endpoint.py 92.77% <100%> (+0.04%) ⬆️
aiobotocore/__init__.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aef7799...008a313. Read the comment docs.

@Gr1N Gr1N mentioned this pull request Feb 27, 2018
setup.py Outdated
'multidict>=2.1.4',
'wrapt>=1.10.10',
'packaging>=16.8',
]

PY_VER = sys.version_info

if not PY_VER >= (3, 4, 1):
raise RuntimeError("aiobotocore doesn't support Python earlier than 3.4")
if not PY_VER >= (3, 5, 0):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3.5.3 actually. The same in all other places

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, thanks


# for our patch of _wait
StreamReader: {'c0a9a31a8c3e550de5985ab642028983f709b37b'},
StreamReader: {'d4ffb6ae823ef4bfd810aade8601ba7b01aa08ec'},

# for digging into _protocol ( 2.1.x, 2.2.x )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can remove 2.1.x, 2.2.x

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@thehesiod
Copy link
Collaborator

cool looks good! I validated the changes to aiobotocore and looks like there's no impact to us, yay. just have one comment to remove a comment :) Let's see what @jettify says too.

@thehesiod
Copy link
Collaborator

would be good to bump to the latest botocore too, but we can do that in another PR

# `self._loop` explicitly because from `aiohttp>=3.0.0` we can't
# pass `loop` as a kwargs into `run_app`.
with mock.patch('asyncio.get_event_loop', return_value=self._loop):
aiohttp.web.run_app(app, host=host, port=self._port,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use new AppRunner stuff as result we do not need ugly patch
https://docs.aiohttp.org/en/stable/web_advanced.html?highlight=AppRunner#application-runners

@jettify
Copy link
Member

jettify commented Feb 27, 2018

Changes looks good to me as well, one nitpick. We can merge PR as it is and handle runner stuff in separate PR

@Gr1N
Copy link
Contributor Author

Gr1N commented Feb 28, 2018

@jettify maybe it's bad, but I don't want to spend time on AppRunner :). If it possible please merge this PR and migrate to AppRunner in separate PR.

@jettify jettify merged commit b24e38e into aio-libs:master Feb 28, 2018
@jettify
Copy link
Member

jettify commented Feb 28, 2018

thanks!

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.

5 participants