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

Isotp typing [ready] #3078

Merged
merged 15 commits into from
Apr 22, 2021
Merged

Isotp typing [ready] #3078

merged 15 commits into from
Apr 22, 2021

Conversation

polybassa
Copy link
Contributor

No description provided.

@polybassa polybassa force-pushed the isotp_typing branch 2 times, most recently from 8f97cf5 to 03ffb68 Compare February 1, 2021 10:17
@polybassa polybassa mentioned this pull request Feb 1, 2021
@polybassa polybassa marked this pull request as ready for review February 1, 2021 13:06
@codecov
Copy link

codecov bot commented Feb 1, 2021

Codecov Report

Merging #3078 (5dec200) into master (18b71dd) will decrease coverage by 0.01%.
The diff coverage is 65.26%.

@@            Coverage Diff             @@
##           master    #3078      +/-   ##
==========================================
- Coverage   85.32%   85.30%   -0.02%     
==========================================
  Files         262      262              
  Lines       54731    54742      +11     
==========================================
+ Hits        46697    46699       +2     
- Misses       8034     8043       +9     
Impacted Files Coverage Δ
scapy/compat.py 95.42% <ø> (ø)
scapy/contrib/automotive/kwp.py 95.34% <ø> (ø)
scapy/fields.py 91.38% <ø> (ø)
scapy/contrib/isotp.py 69.17% <62.98%> (-0.63%) ⬇️
scapy/sessions.py 94.85% <66.66%> (+1.07%) ⬆️
scapy/contrib/automotive/gm/gmlanutils.py 90.00% <100.00%> (+1.73%) ⬆️
scapy/arch/windows/__init__.py 67.73% <0.00%> (-0.57%) ⬇️

Copy link
Member

@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.

I'll likely go over it again. I expect this to break when the sndrcv PR is merged.

scapy/sessions.py Outdated Show resolved Hide resolved
scapy/contrib/isotp.py Outdated Show resolved Hide resolved
scapy/contrib/isotp.py Outdated Show resolved Hide resolved
scapy/contrib/isotp.py Outdated Show resolved Hide resolved
scapy/contrib/isotp.py Outdated Show resolved Hide resolved
scapy/contrib/isotp.py Outdated Show resolved Hide resolved
scapy/contrib/isotp.py Outdated Show resolved Hide resolved
scapy/contrib/isotp.py Outdated Show resolved Hide resolved
@gpotter2 gpotter2 added PEPin Apply PEP08 rules Hinty and removed PEPin Apply PEP08 rules labels Feb 2, 2021
@polybassa
Copy link
Contributor Author

I'll likely go over it again. I expect this to break when the sndrcv PR is merged.

Yes, I saw a couple of nice changes in sendrcv core typing. Looking forward to have them in.

@polybassa
Copy link
Contributor Author

waiting for #3059

@polybassa
Copy link
Contributor Author

@gpotter2 Rebased and ready for merge

@polybassa
Copy link
Contributor Author

@gpotter2 Rebased.

scapy/contrib/automotive/gm/gmlanutils.py Outdated Show resolved Hide resolved
scapy/contrib/automotive/gm/gmlanutils.py Outdated Show resolved Hide resolved
scapy/contrib/automotive/gm/gmlanutils.py Outdated Show resolved Hide resolved
scapy/contrib/automotive/gm/gmlanutils.py Outdated Show resolved Hide resolved
scapy/contrib/isotp.py Outdated Show resolved Hide resolved
scapy/contrib/isotp.py Show resolved Hide resolved
scapy/contrib/isotp.py Outdated Show resolved Hide resolved
scapy/contrib/isotp.py Outdated Show resolved Hide resolved
scapy/contrib/isotp.py Outdated Show resolved Hide resolved
scapy/contrib/isotp.py Outdated Show resolved Hide resolved
@polybassa
Copy link
Contributor Author

Thanks for your feedback.. I will apply it :-)

@polybassa
Copy link
Contributor Author

Applied all feedback, except recv_raw related comments. Is it possible to break the return type of mypy into multiple lines?

@polybassa polybassa closed this Mar 17, 2021
@polybassa polybassa reopened this Mar 17, 2021
@polybassa polybassa closed this Mar 17, 2021
@polybassa polybassa changed the title Isotp typing Isotp typing [ready] Mar 22, 2021
@gpotter2
Copy link
Member

gpotter2 commented Mar 29, 2021

Applied all feedback, except recv_raw related comments. Is it possible to break the return type of mypy into multiple lines?

Not using Python 2 typing format. That's annoying for sure

@gpotter2 gpotter2 added this to the 2.5.0 milestone Mar 29, 2021
@polybassa
Copy link
Contributor Author

Hi, thanks for this feedback. No worries ;-)
So, just to get your request right.

At the moment, the code looks like this:

    def recv_raw(self, x=MTU):
        # type: (int) -> Tuple[Optional[Type[Packet]], Optional[bytes], Optional[float]]  # noqa: E501

scapy/scapy/supersocket.py

Lines 166 to 167 in 4bec6ee

def recv_raw(self, x=MTU):
# type: (int) -> Tuple[Optional[Type[Packet]], Optional[bytes], Optional[float]] # noqa: E501

And you request to change it to

    def recv_raw(self,
                          x=MTU  # type: int
                          ):
        # type: (...) -> Tuple[Optional[Type[Packet]], Optional[bytes], Optional[float]]  # noqa: E501

I just wanted to get this right, since I thought the intention of the multiline typing is to save noqa's, which would not be the case in this change.

Thanks for your support!

I've resend this answer to have markdown support.
@gpotter2 Could you please give me feedback on this question

gpotter2
gpotter2 previously approved these changes Apr 19, 2021
Copy link
Member

@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.

Conflicts otherwise lgtm

@gpotter2
Copy link
Member

Sorry for the late answer polybassa, to answer your question, you are right that in this case (return type takes most of the space) it makes more sense to stay single line.

I've fixed the conflicts

@polybassa polybassa closed this Apr 20, 2021
@polybassa polybassa reopened this Apr 20, 2021
@gpotter2 gpotter2 merged commit 1244554 into secdev:master Apr 22, 2021
bzalkilani pushed a commit to bzalkilani/scapy that referenced this pull request Jun 12, 2022
@polybassa polybassa deleted the isotp_typing branch July 27, 2022 08:59
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.

2 participants