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 some low-effort type annotations #2477

Merged
merged 14 commits into from
Jan 17, 2023
Merged

Conversation

harahu
Copy link
Contributor

@harahu harahu commented Nov 16, 2022

This PR adds some uncontroversial annotations to the central trio.run method. From my perspective, it is mainly intended as an experiment into whether a gradual, opportunistic approach to adding types to this project is feasible.

@codecov
Copy link

codecov bot commented Nov 16, 2022

Codecov Report

Merging #2477 (fc4ed29) into master (af3d7d8) will increase coverage by 1.04%.
The diff coverage is 94.11%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2477      +/-   ##
==========================================
+ Coverage   92.46%   93.50%   +1.04%     
==========================================
  Files         118      118              
  Lines       16327    16336       +9     
  Branches     3156     3157       +1     
==========================================
+ Hits        15096    15275     +179     
+ Misses       1103      955     -148     
+ Partials      128      106      -22     
Impacted Files Coverage Δ
trio/_core/_run.py 98.92% <94.11%> (+0.22%) ⬆️
trio/tests/test_subprocess.py 96.96% <0.00%> (+0.55%) ⬆️
trio/_socket.py 95.68% <0.00%> (+0.78%) ⬆️
trio/_core/tests/test_io.py 99.29% <0.00%> (+1.05%) ⬆️
trio/tests/test_socket.py 98.21% <0.00%> (+1.13%) ⬆️
trio/tests/test_threads.py 100.00% <0.00%> (+1.56%) ⬆️
trio/_highlevel_socket.py 98.19% <0.00%> (+1.80%) ⬆️
trio/tests/test_highlevel_open_tcp_stream.py 100.00% <0.00%> (+1.83%) ⬆️
trio/_subprocess.py 93.91% <0.00%> (+2.17%) ⬆️
... and 8 more

@pquentin
Copy link
Member

Thank you for reviving this effort! Previous attempts are https://github.com/python-trio/trio-typing, #1873 and #2208.

A gradual approach is certainly possible: https://sethmlarson.dev/blog/tests-arent-enough-case-study-after-adding-types-to-urllib3. But I think you have to make it work file by file using an allowlist, as done for urllib3. Case in point, the mypy check is failing for this pull request.

@harahu
Copy link
Contributor Author

harahu commented Nov 16, 2022

@pquentin We can probably figure out the exact mypy config as we go.

The reason it failed was due to the check_untyped_defs = False option being set. The moment I started adding types to a few previously untyped functions, I uncovered issues hidden in said functions. This is probably fine, though, as the alternative would be pretty noisy at the moment.

I've made mypy succeed now, but I ended up adding some uncovered lines in the process.

mypy.ini Outdated Show resolved Hide resolved
trio/_core/_run.py Outdated Show resolved Hide resolved
trio/_core/_run.py Outdated Show resolved Hide resolved
Copy link
Contributor

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

The reordering of imports is unfortunate, but this looks good overall.

Unfortunately compatibility with typing is a bit cursed: from __future__ import annotations is in limbo (note the lack of resolution for 3.12). If that ever gets removed, we need to make sure we use that version's types at runtime -- of course, it shouldn't be. It's likely that we'll have to change it to co_annotations or similar soon. We'll just have to take this as we can. A good next step would be adding mypy to ci.sh to run it on every platform we test (other than PyPy ones, of course).

@harahu harahu merged commit d61b050 into python-trio:master Jan 17, 2023
@harahu harahu deleted the run-types branch January 17, 2023 13:35
@Zac-HD Zac-HD mentioned this pull request Jul 21, 2023
11 tasks
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.

4 participants