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

[Hinty] Type hinting: prepare tests & setup Mypy #2162

Merged
merged 2 commits into from
Aug 27, 2019
Merged

Conversation

gpotter2
Copy link
Member

@gpotter2 gpotter2 commented Jul 22, 2019

This PR provides the tools, scripts and tests required to start "Project Hinty" = adding type hinting into Scapy.

  • provides UTscapy util to use pyannotate to generate type hinting
  • add Mypy tests to check regressions
  • relocate test configs
  • some various fixes that were failing on my test machine
  • two files were used (processed) as a trial: main.py and __init__.py. Because http2.py had also been done previously, it was also included.

@codecov
Copy link

codecov bot commented Jul 22, 2019

Codecov Report

Merging #2162 into master will decrease coverage by 0.8%.
The diff coverage is 59.32%.

@@            Coverage Diff             @@
##           master    #2162      +/-   ##
==========================================
- Coverage   84.78%   83.97%   -0.81%     
==========================================
  Files         202      194       -8     
  Lines       46098    44686    -1412     
==========================================
- Hits        39082    37525    -1557     
- Misses       7016     7161     +145
Impacted Files Coverage Δ
scapy/__init__.py 84.09% <100%> (+0.36%) ⬆️
scapy/contrib/http2.py 96.79% <100%> (+1.29%) ⬆️
scapy/compat.py 79.22% <11.11%> (-20.78%) ⬇️
scapy/main.py 72.75% <73.33%> (-1.05%) ⬇️
scapy/layers/tftp.py 37.13% <0%> (-51.15%) ⬇️
scapy/arch/common.py 43.33% <0%> (-46.67%) ⬇️
scapy/layers/dhcp6.py 64.18% <0%> (-21.56%) ⬇️
scapy/arch/__init__.py 75.55% <0%> (-20%) ⬇️
scapy/supersocket.py 53.4% <0%> (-18.19%) ⬇️
scapy/consts.py 79.16% <0%> (-12.5%) ⬇️
... and 56 more

@gpotter2 gpotter2 changed the title [cool project name] Type hinting: prepare tests & setup Mypy [Hinty] Type hinting: prepare tests & setup Mypy Jul 23, 2019
Copy link
Member

@guedou guedou left a comment

Choose a reason for hiding this comment

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

Really cool PR!

See my comments/remarks.

.mypy/mypy_check.py Outdated Show resolved Hide resolved
.mypy/mypy_check.py Outdated Show resolved Hide resolved
scapy/__init__.py Outdated Show resolved Hide resolved
scapy/__init__.py Outdated Show resolved Hide resolved
scapy/__init__.py Outdated Show resolved Hide resolved
scapy/main.py Outdated Show resolved Hide resolved
scapy/tools/UTscapy.py Show resolved Hide resolved
@gpotter2
Copy link
Member Author

gpotter2 commented Jul 23, 2019

This is going to be quite some work. After having done main.py and __init__.py as a starter to test this PR, we'll probably have to do most files one by one (one per PR).

@guedou Below is a guide on how to contribute. Should we put it somewhere?

Guide on how to contribute

  1. Pick a file you want to work on
  2. Run the tests with UTscapy with the -x option enabled. (this requires to have pyannotate installed) This will create test/pyannotate_results
  3. pyannotate --type-info test/pyannotate_results -w [the file.py you are working on]
  4. The file has been automatically processed. Now, edit it to fix the mistakes, and check your work with tox -e mypy. Remember that you can use cast(type, obj) from typing to help mypy without affecting the code.
  5. If you get error: No library stub file for module ..., add an exception for it in .config/mypy/mypy.ini (follow the existing format)
  6. Check that the files still pass PEP8: tox -e flake8
  7. When everything passes, submit a PR. 😄

@gpotter2
Copy link
Member Author

Another note: typing isn't installed on Python 2.7 (but is on Python 3.5+).
This adds a dependency to Scapy for Python 2.7. Scapy will still be able to be ran "out-of-the-box" for Python 3.5+.

Should we then choose to remove run_scapy_py2.bat ?

@gpotter2 gpotter2 force-pushed the mypy branch 5 times, most recently from abd5875 to 4fd233a Compare July 24, 2019 00:39
Copy link
Member

@guedou guedou left a comment

Choose a reason for hiding this comment

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

See my comments.

.config/travis/install.sh Show resolved Hide resolved
.config/travis/install.sh Outdated Show resolved Hide resolved
scapy/__init__.py Outdated Show resolved Hide resolved
scapy/main.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
scapy/tools/UTscapy.py Outdated Show resolved Hide resolved
@guedou
Copy link
Member

guedou commented Jul 24, 2019

@gpotter2 the guide can be integrated into CONTRIBUTING.md

@guedou
Copy link
Member

guedou commented Jul 24, 2019

BTW, http2.py already uses typing.

@guedou
Copy link
Member

guedou commented Jul 24, 2019

BTW, can you remove line 2502 from scapy/contrib/http2.py ?

@gpotter2
Copy link
Member Author

gpotter2 commented Jul 24, 2019

I think I've addressed all your points. I've also added http2.py to the list of files that get checked (with a few tweaks).

Also, it's up to you to merge that before 2.4.3. As we don't require typing, it shouldn't have much effect...

@gpotter2 gpotter2 force-pushed the mypy branch 2 times, most recently from 285d0c9 to 1177ec4 Compare July 24, 2019 18:09
.config/mypy/mypy.ini Outdated Show resolved Hide resolved
@gpotter2
Copy link
Member Author

Should be ready to merge

p-l-
p-l- previously approved these changes Aug 22, 2019
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.

This is an awesome work again @gpotter2.
Could you rebase against current master?

@gpotter2
Copy link
Member Author

Rebased

@p-l-
Copy link
Member

p-l- commented Aug 26, 2019

Your call, @guedou.

@guedou
Copy link
Member

guedou commented Aug 27, 2019

@gpotter2 could you open an issue like #1277 to track the progress of adding types to Scapy?

@guedou guedou merged commit 99dbbdd into secdev:master Aug 27, 2019
@gpotter2
Copy link
Member Author

This was supposed to be #2158.

@guedou
Copy link
Member

guedou commented Aug 28, 2019 via email

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.

3 participants