Skip to content

Add types to _unix_pipes#2502

Merged
harahu merged 1 commit intopython-trio:masterfrom
harahu:types-unix-pipes
Dec 13, 2022
Merged

Add types to _unix_pipes#2502
harahu merged 1 commit intopython-trio:masterfrom
harahu:types-unix-pipes

Conversation

@harahu
Copy link
Contributor

@harahu harahu commented Dec 13, 2022

Complete type annotations for the _unix_pipes module.

@codecov
Copy link

codecov bot commented Dec 13, 2022

Codecov Report

Merging #2502 (f207b2b) into master (52eab84) will increase coverage by 0.86%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2502      +/-   ##
==========================================
+ Coverage   97.98%   98.84%   +0.86%     
==========================================
  Files         118      118              
  Lines       16324    16327       +3     
  Branches     3156     3156              
==========================================
+ Hits        15995    16139     +144     
+ Misses        266      133     -133     
+ Partials       63       55       -8     
Impacted Files Coverage Δ
trio/_unix_pipes.py 100.00% <100.00%> (ø)
trio/_core/_run.py 99.45% <0.00%> (+0.43%) ⬆️
trio/tests/test_socket.py 99.83% <0.00%> (+0.64%) ⬆️
trio/_core/tests/test_io.py 100.00% <0.00%> (+1.05%) ⬆️
trio/tests/test_highlevel_open_tcp_stream.py 100.00% <0.00%> (+1.47%) ⬆️
trio/_subprocess_platform/__init__.py 100.00% <0.00%> (+5.71%) ⬆️
trio/_core/__init__.py 100.00% <0.00%> (+12.50%) ⬆️
trio/lowlevel.py 100.00% <0.00%> (+16.66%) ⬆️
trio/_core/_io_kqueue.py 84.67% <0.00%> (+83.87%) ⬆️
trio/_subprocess_platform/kqueue.py 100.00% <0.00%> (+100.00%) ⬆️

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good, but how did you test it? Is running mypy on this file working? If so we should do it in CI too.

@harahu
Copy link
Contributor Author

harahu commented Dec 13, 2022

This looks good, but how did you test it? Is running mypy on this file working?

@pquentin I tested it by running mypy on the file. It is working.

If so we should do it in CI too.

We already are, for python 3.8 at least: https://github.com/python-trio/trio/actions/runs/3683469595/jobs/6232645740

(Look at the bottom of the "Run tests" step)

@harahu harahu merged commit 59f2eb3 into python-trio:master Dec 13, 2022
@harahu harahu deleted the types-unix-pipes branch December 13, 2022 10:01
@pquentin
Copy link
Member

This makes sure that the annotations are valid, but not that they are complete. If tomorrow I add a new function to this file without annotations mypy won't complain about it.

It's not blocking for this pull request but we should strive to have complete annotations for each file and enforce that in CI using mypy --strict. This approach was used successfully by urllib3.

@harahu
Copy link
Contributor Author

harahu commented Dec 13, 2022

It's not blocking for this pull request but we should strive to have complete annotations for each file and enforce that in CI using mypy --strict. This approach was used successfully by urllib3.

I'm aware that this technique has been used with success, and I'm a fan of introducing it. Doing so should be a separate PR. I might look into it soon.

For now, though, annotation coverage is so low that regressing coverage is the least of my worries.

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