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

Enable Python 3.8 coverage #1339

Merged
merged 2 commits into from
Feb 3, 2020
Merged

Conversation

pquentin
Copy link
Member

We now test Python 3.8 on Windows, so I rebased the commits of #784 on top of master (without using coverage master). I don't know how to configure Azure Pipelines on my fork, so here's a PR. It's only a draft because I first want to see if we see the same things.

@codecov
Copy link

codecov bot commented Dec 16, 2019

Codecov Report

Merging #1339 into master will increase coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1339      +/-   ##
==========================================
+ Coverage   99.52%   99.71%   +0.19%     
==========================================
  Files         106      106              
  Lines       13009    13318     +309     
  Branches      996     1015      +19     
==========================================
+ Hits        12947    13280     +333     
+ Misses         43       23      -20     
+ Partials       19       15       -4     
Impacted Files Coverage Δ
trio/_abc.py 100.00% <0.00%> (ø) ⬆️
trio/_ssl.py 100.00% <0.00%> (ø) ⬆️
trio/_path.py 100.00% <0.00%> (ø) ⬆️
trio/_sync.py 100.00% <0.00%> (ø) ⬆️
trio/_channel.py 100.00% <0.00%> (ø) ⬆️
trio/_file_io.py 100.00% <0.00%> (ø) ⬆️
trio/_signals.py 100.00% <0.00%> (ø) ⬆️
trio/_threads.py 100.00% <0.00%> (ø) ⬆️
trio/_timeouts.py 100.00% <0.00%> (ø) ⬆️
trio/_deprecate.py 100.00% <0.00%> (ø) ⬆️
... and 45 more

@pquentin
Copy link
Member Author

We could wait to re-enable coverage until 3.8 is actually released. That would at least handle the fromshare issue...

Originally posted by @njsmith in #784 (comment)

Indeed, we get the fromshare fix for free. However, that does not fix the aiter_compat issue that we use for 3.5.0 and 3.5.1. I don't really know how to fix it. Here is the full code:

trio/trio/_util.py

Lines 72 to 83 in d47e018

if sys.version_info < (3, 5, 2):
def aiter_compat(aiter_impl):
@wraps(aiter_impl)
async def __aiter__(*args, **kwargs):
return aiter_impl(*args, **kwargs)
return __aiter__
else:
def aiter_compat(aiter_impl):
return aiter_impl

I don't even understand how that works. The idea is to fix https://bugs.python.org/issue27243, but with something more generic than aio-libs/aiohttp@7276cc9. I cant't use a if inside the def since we want to define the functions in both case. And I tried to desugar but it didn't work.

I'm sure @njsmith can come up with a clever fix but I'd rather drop support for Python 3.5 as I think we reached consensus in #75.

Copy link
Member

@njsmith njsmith left a comment

Choose a reason for hiding this comment

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

Two comments, neither of which necessarily requires action now...

Want to make a PR to deprecate python 3.5 support?

# Disable coverage on 3.8 until we run 3.8 on Windows CI too
# https://github.com/python-trio/trio/pull/784#issuecomment-446438407
if [[ "$(python -V)" = Python\ 3.8* ]]; then
true;
# coverage is broken in pypy3 7.1.1, but is fixed in nightly and should be
# fixed in the next release after 7.1.1.
# See: https://bitbucket.org/pypy/pypy/issues/2943/
Copy link
Member

Choose a reason for hiding this comment

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

I guess 7.2.0 is out, so possibly we can remove this too...

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to try that next. 👍

trio/_util.py Outdated
async def __aiter__(*args, **kwargs):
return aiter_impl(*args, **kwargs)
__aiter__ = wraps(aiter_impl)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
__aiter__ = wraps(aiter_impl)
__aiter__ = wraps(aiter_impl)(__aiter__)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I just did that. I'll worry about dropping 3.5 in another pull request.

Python 3.8 considers the `async def __aiter(*args, **kwargs)` line as a
line to be covered, but as this code is not run under Python 3.8, it's
not considered covered. Using the explicit decorator syntax fixes the
issue.
@pquentin pquentin marked this pull request as ready for review December 17, 2019 07:14
@pquentin
Copy link
Member Author

python-trio.trio Expected — Waiting for status to be reported

Not sure why it's not reported. Closing/reopening.

@pquentin pquentin closed this Dec 17, 2019
@pquentin pquentin reopened this Dec 17, 2019
@pquentin
Copy link
Member Author

Closing/reopening to see if I get python-trio.trio check this time

@pquentin pquentin closed this Jan 30, 2020
@pquentin pquentin reopened this Jan 30, 2020
@pquentin
Copy link
Member Author

pquentin commented Feb 3, 2020

@njsmith Could you please take another look? Thanks!

@njsmith njsmith merged commit 1794202 into python-trio:master Feb 3, 2020
@pquentin pquentin deleted the coverage-py38 branch February 3, 2020 10:33
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.

2 participants