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 typing for _highlevel_open_tcp_listeners.py #2724

Merged
merged 27 commits into from
Aug 6, 2023

Conversation

CoolCat467
Copy link
Member

This PR adds type annotations to _highlevel_open_tcp_listeners.py.

The only thing that might be a bit off is the type of task_status in the serve_tcp function. I annotated it as trio.lowlevel.Task, but I am not sure this is right.

I would love feedback and any comments on how this could be improved to be more accurate.

@codecov
Copy link

codecov bot commented Jul 29, 2023

Codecov Report

Merging #2724 (2ab89e4) into master (348713a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2724   +/-   ##
=======================================
  Coverage   98.90%   98.90%           
=======================================
  Files         113      113           
  Lines       16669    16683   +14     
  Branches     3021     3025    +4     
=======================================
+ Hits        16487    16501   +14     
  Misses        125      125           
  Partials       57       57           
Files Changed Coverage Δ
trio/__init__.py 100.00% <ø> (ø)
trio/_dtls.py 96.59% <ø> (ø)
trio/lowlevel.py 100.00% <ø> (ø)
trio/_highlevel_open_tcp_listeners.py 100.00% <100.00%> (ø)
trio/_tests/test_highlevel_open_tcp_listeners.py 98.32% <100.00%> (+0.06%) ⬆️

@A5rocks A5rocks added the typing Adding static types to trio's interface label Jul 29, 2023
@A5rocks
Copy link
Contributor

A5rocks commented Jul 30, 2023

FYI I think verify_types.json is messed up a bit. Maybe your local env is a bit strange?

Copy link
Contributor

@TeamSpen210 TeamSpen210 left a comment

Choose a reason for hiding this comment

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

A few things that aren't quite right. verify_types.json is a bit picky, it doesn't give the same results for me either.

trio/_highlevel_open_tcp_listeners.py Show resolved Hide resolved
trio/_highlevel_open_tcp_listeners.py Outdated Show resolved Hide resolved
trio/_highlevel_open_tcp_listeners.py Outdated Show resolved Hide resolved
@jakkdl
Copy link
Member

jakkdl commented Jul 30, 2023

FYI I think verify_types.json is messed up a bit. Maybe your local env is a bit strange?

#2699 tox would help here.
Getting the environment right can actually be quite tricky IME

Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

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

Update pyproject.toml to add this file to the list of files that has increased strictness.

@CoolCat467
Copy link
Member Author

Interesting, Windows pypy 3.9 nightly x64 failed with a segfault from the tests script: https://github.com/python-trio/trio/actions/runs/5705575738/job/15460400267?pr=2724

@CoolCat467 CoolCat467 requested a review from jakkdl July 30, 2023 10:38
@jakkdl
Copy link
Member

jakkdl commented Jul 30, 2023

Interesting, Windows pypy 3.9 nightly x64 failed with a segfault from the tests script: https://github.com/python-trio/trio/actions/runs/5705575738/job/15460400267?pr=2724

it does that every now and then in my experience - no clue why. It's always been fixed by rerunning for me, but somebody should maybe open an issue and/or try to investigate the flakiness.

Copy link
Member

@jakkdl jakkdl left a comment

Choose a reason for hiding this comment

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

Task -> TaskStatus as TeamSpen pointed out, but otherwise mostly good. No need for a re-review from me.

trio/_highlevel_open_tcp_listeners.py Outdated Show resolved Hide resolved
@CoolCat467 CoolCat467 requested a review from jakkdl August 1, 2023 16:23
@CoolCat467 CoolCat467 requested a review from A5rocks August 2, 2023 04:50
Copy link
Contributor

@TeamSpen210 TeamSpen210 left a comment

Choose a reason for hiding this comment

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

Looks, good, except for one minor quibble.


import trio

from . import socket as tsocket

if TYPE_CHECKING:
from trio.lowlevel import TaskStatus
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this import need to be guarded (due to import cycles), or can it be a real import? It's probably better to just import if we can, so that this can be evaluated at runtime.

Copy link
Member Author

@CoolCat467 CoolCat467 Aug 2, 2023

Choose a reason for hiding this comment

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

Does not need to be guarded as far as I am aware

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted in 4a9b024

Copy link
Member

Choose a reason for hiding this comment

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

#2687

This is kinda up in the air, I'd love it if we definitively resolved it and added a CI check that made usage consistent and saved review cycles. https://pypi.org/project/flake8-type-checking/

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 would suggest using https://pypi.org/project/ruff/ instead, it does the same thing and more but faster and with auto-fix support

@jakkdl jakkdl merged commit 49d0393 into python-trio:master Aug 6, 2023
27 checks passed
@jakkdl
Copy link
Member

jakkdl commented Aug 6, 2023

Given the number of open typing PRs, I'm merging this one. Any minor problems can be addressed later if you want to bother going through it @A5rocks

@CoolCat467 CoolCat467 deleted the typing-highlevel-tcp branch August 6, 2023 21:52
@CoolCat467 CoolCat467 restored the typing-highlevel-tcp branch August 7, 2023 15:26
@CoolCat467 CoolCat467 deleted the typing-highlevel-tcp branch August 7, 2023 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typing Adding static types to trio's interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants