Skip to content

Core typing: sendrecv.py #3059

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

Merged
merged 3 commits into from
Feb 17, 2021
Merged

Core typing: sendrecv.py #3059

merged 3 commits into from
Feb 17, 2021

Conversation

gpotter2
Copy link
Member

@gpotter2 gpotter2 commented Jan 15, 2021

This PR:

  • adds core typing to sendrecv.py

Needs: #3055

This is one of the last extremly big & annoying PRs of this kind. There are several parts left but none this central.

@gpotter2 gpotter2 added the Hinty label Jan 15, 2021
@gpotter2 gpotter2 changed the title Mypy sendrecv Core typing: sendrecv.py Jan 15, 2021
@gpotter2 gpotter2 force-pushed the mypy-sendrecv branch 3 times, most recently from fb970b3 to b6c19ac Compare January 16, 2021 13:47
@gpotter2 gpotter2 force-pushed the mypy-sendrecv branch 10 times, most recently from 6e04815 to b645b93 Compare January 29, 2021 12:18
@gpotter2 gpotter2 marked this pull request as ready for review January 29, 2021 12:27
@codecov
Copy link

codecov bot commented Jan 29, 2021

Codecov Report

Merging #3059 (a1066ea) into master (22cd038) will decrease coverage by 0.00%.
The diff coverage is 83.08%.

@@            Coverage Diff             @@
##           master    #3059      +/-   ##
==========================================
- Coverage   85.36%   85.35%   -0.01%     
==========================================
  Files         256      256              
  Lines       54191    54226      +35     
==========================================
+ Hits        46258    46283      +25     
- Misses       7933     7943      +10     
Impacted Files Coverage Δ
scapy/base_classes.py 79.75% <20.00%> (-0.25%) ⬇️
scapy/utils.py 78.42% <68.00%> (+0.31%) ⬆️
scapy/supersocket.py 60.12% <83.33%> (-0.07%) ⬇️
scapy/sendrecv.py 83.89% <87.67%> (-0.67%) ⬇️
scapy/compat.py 95.10% <100.00%> (+0.17%) ⬆️
scapy/interfaces.py 95.38% <100.00%> (+0.02%) ⬆️
scapy/layers/l2.py 76.17% <100.00%> (ø)
scapy/main.py 70.87% <100.00%> (ø)
scapy/packet.py 82.78% <100.00%> (+0.01%) ⬆️
scapy/plist.py 86.31% <100.00%> (+0.10%) ⬆️
... and 4 more

Copy link
Member Author

@gpotter2 gpotter2 left a comment

Choose a reason for hiding this comment

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

This is ready for review. Among other things, this PR discovered:

  • fix "Bad number of arguments" type of issues on PcapReader & cie.
  • type overloading of some utils: remove several cast
  • fix *args being misplaced (x2)
  • add some doc for sendrecv functions
  • fix select_func being called with inconsistent types (dict or lists)
  • fix an edge case in AsyncSniffer which could result in self.thread.start() being called with self.thread = None
  • remove unused variables in some sendrecv functions
  • remove compat.py from sphinx generation: see the commit for an explanation. Basically there's a builtin docstring in the typing module that is wrong, and that we can't fix.

This however still requires #3055 to be merged first

Comment on lines +166 to +169
@overload
def get_temp_file(keep, autoext, fd):
# type: (bool, str, Literal[True]) -> IO[bytes]
pass
Copy link
Member Author

Choose a reason for hiding this comment

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

This is pretty cool

Copy link
Member

Choose a reason for hiding this comment

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

Agree!

Sphinx raises a warning on `typing.overload` that we cannot fix,
because it's related to a dependency that is builtin :/
@gpotter2 gpotter2 force-pushed the mypy-sendrecv branch 4 times, most recently from b645b93 to a1066ea Compare January 30, 2021 23:21
Copy link
Contributor

@polybassa polybassa left a comment

Choose a reason for hiding this comment

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

I'll carefully read through this PR. I couldn't find any issue.

@gpotter2
Copy link
Member Author

gpotter2 commented Feb 6, 2021

@guedou @p-l- Hi, this is ready for review. (btw It appears we're currently having an issue in our tests related to OpenSSL versions.)

Copy link
Member

@p-l- p-l- left a comment

Choose a reason for hiding this comment

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

LGTM, great work again!

@gpotter2
Copy link
Member Author

@guedou Your call

@guedou guedou merged commit 488794c into secdev:master Feb 17, 2021
@gpotter2 gpotter2 deleted the mypy-sendrecv branch February 17, 2021 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants