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

Fix type hint for tracing Signal sequence content #3596

Merged
merged 6 commits into from
May 8, 2019

Conversation

stj
Copy link
Contributor

@stj stj commented Feb 6, 2019

The Signal is instantiated with a TraceConfig object but does not store
it in its mutable sequence. Fixed hinting to reflect what is expected to
be added as signal handler.

What do these changes do?

Fix type hint for tracing Signal contents. They got very long with the last argument being a Union, maybe an Any would do instead?

Are there changes in behavior for the user?

mypy does not report an error of
error: List item 0 has incompatible type "Callable[[ClientSession, SimpleNamespace, TraceConnectionCreateEndParams], Coroutine[Any, Any, None]]"; expected "Callable[[TraceConfig], Awaitable[None]]"

Related issue number

#3595

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • Add a new news fragment into the CHANGES folder

Sorry, something went wrong.

@stj stj requested a review from asvetlov as a code owner February 6, 2019 00:27
@codecov-io
Copy link

codecov-io commented Feb 6, 2019

Codecov Report

Merging #3596 into master will decrease coverage by 3.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3596      +/-   ##
==========================================
- Coverage    97.9%   94.83%   -3.08%     
==========================================
  Files          43       43              
  Lines        8556     8555       -1     
  Branches     1376     1375       -1     
==========================================
- Hits         8377     8113     -264     
- Misses         74      337     +263     
  Partials      105      105
Impacted Files Coverage Δ
aiohttp/web_urldispatcher.py 98.96% <ø> (-0.3%) ⬇️
aiohttp/helpers.py 96.46% <ø> (-0.76%) ⬇️
aiohttp/tracing.py 100% <100%> (ø) ⬆️
aiohttp/worker.py 6.55% <0%> (-90.99%) ⬇️
aiohttp/resolver.py 46.55% <0%> (-53.45%) ⬇️
aiohttp/web_fileresponse.py 66.66% <0%> (-29.89%) ⬇️
aiohttp/tcp_helpers.py 63.33% <0%> (-26.67%) ⬇️
aiohttp/locks.py 92.85% <0%> (-7.15%) ⬇️
aiohttp/web_runner.py 91.7% <0%> (-6.22%) ⬇️
aiohttp/__init__.py 94.44% <0%> (-5.56%) ⬇️
... and 8 more

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 36331ce...ab5683c. Read the comment docs.

@stj stj force-pushed the fix-typing-for-signal-contents branch from d401685 to 1d6e774 Compare February 6, 2019 05:33

Unverified

No user is associated with the committer email.
The Signal is instantiated with a TraceConfig object but does not store
it in its mutable sequence. Fixed hinting to reflect what is expected to
be added as signal handler.
@stj stj force-pushed the fix-typing-for-signal-contents branch from 1d6e774 to dbd4ae6 Compare February 6, 2019 16:20
@kxepal
Copy link
Member

kxepal commented Feb 14, 2019

Suddenly, this patch doesn't fixes the issue. I run it against your snippet and still get an error:

(venv) ~/projects/aio-libs/aiohttp (master) $ cat bug-3595.py 
import types

import aiohttp
import datadog

async def reuse_keepalive_connection(
    session: aiohttp.ClientSession,
    trace_config_ctx: types.SimpleNamespace,
    params: aiohttp.TraceConnectionReuseconnParams,
) -> None:
    """statsd counter; how often has a connection been reused"""
    datadog.statsd.increment("client.request.connection.reuse")

tracer = aiohttp.TraceConfig()
tracer.on_connection_reuseconn.extend([reuse_keepalive_connection])

(venv) ~/projects/aio-libs/aiohttp (master) $ mypy --ignore-missing-imports bug-3595.py 
bug-3595.py:15: error: List item 0 has incompatible type "Callable[[ClientSession, SimpleNamespace, TraceConnectionReuseconnParams], Coroutine[Any, Any, None]]"; expected "Callable[[ClientSession, SimpleNamespace, <union: 15 items>], None]"

@stj
Copy link
Contributor Author

stj commented Feb 14, 2019

I think I see the problem. Looks like I changed the return type of the Callable from Awaitable[None] to None. Will change an test that.

pyup.io bot and others added 3 commits February 14, 2019 21:32

Unverified

No user is associated with the committer email.
* Update cython from 0.29.2 to 0.29.5

* Update cython from 0.29.2 to 0.29.5

* Update cython from 0.29.2 to 0.29.5

* Update pytest from 4.1.1 to 4.2.0

* Update pytest from 4.1.1 to 4.2.0

* Update pytest-mock from 1.10.0 to 1.10.1

* Update trustme from 0.4.0 to 0.5.0 (Refs: aio-libs#3599, aio-libs#3506)

* Update aiodns from 1.1.1 to 1.2.0

* Update uvloop from 0.11.3 to 0.12.0

* Update flake8 from 3.6.0 to 3.7.5

* Update mypy from 0.660 to 0.670

* Update sphinx from 1.8.3 to 1.8.4

* Update cherry_picker from 1.2.1 to 1.2.2

Unverified

No user is associated with the committer email.
Closes aio-libs#3599

Unverified

No user is associated with the committer email.
@stj
Copy link
Contributor Author

stj commented Feb 15, 2019

That's odd. Was sure that this passed for me but I see that it fails now again. I made it really ugly now but it passes. Type hints are still a mystery to me.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@CLAassistant
Copy link

CLAassistant commented May 8, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 5 committers have signed the CLA.

✅ asvetlov
✅ stj
❌ pyup-bot
❌ webknjaz
❌ Stefan Tjarks


Stefan Tjarks seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@asvetlov asvetlov merged commit 44318e4 into aio-libs:master May 8, 2019
@asvetlov
Copy link
Member

asvetlov commented May 8, 2019

thanks!

asvetlov pushed a commit that referenced this pull request May 8, 2019
* Fix type hint for tracing Signal sequence content

The Signal is instantiated with a TraceConfig object but does not store
it in its mutable sequence. Fixed hinting to reflect what is expected to
be added as signal handler.
asvetlov added a commit that referenced this pull request May 8, 2019
asvetlov added a commit that referenced this pull request May 8, 2019
@lock lock bot added the outdated label May 20, 2020
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants