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

Improve API documentation of sndrecv and SuperSocket functions. #4114

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

polybassa
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Sep 11, 2023

Codecov Report

Merging #4114 (81a7975) into master (6829f4c) will increase coverage by 32.30%.
The diff coverage is 100.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #4114       +/-   ##
===========================================
+ Coverage   49.67%   81.98%   +32.30%     
===========================================
  Files         342      349        +7     
  Lines       76267    81853     +5586     
===========================================
+ Hits        37886    67105    +29219     
+ Misses      38381    14748    -23633     
Files Coverage Δ
scapy/sendrecv.py 87.68% <100.00%> (+48.12%) ⬆️
scapy/supersocket.py 75.64% <ø> (ø)

... and 263 files with indirect coverage changes

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.

Thanks, that's a cool PR.

scapy/sendrecv.py Outdated Show resolved Hide resolved
scapy/sendrecv.py Outdated Show resolved Hide resolved
scapy/sendrecv.py Outdated Show resolved Hide resolved
@polybassa
Copy link
Contributor Author

Thanks.. can we merge it?

@gpotter2
Copy link
Member

I'll just try to test it and merge it when it's done, but it looks alright.

I would also like to fix readthedocs builds, which have been broken for a while.

@polybassa
Copy link
Contributor Author

whats broken? Can I assist?

@gpotter2
Copy link
Member

Builds fail (i.e. doc is not updated).

https://readthedocs.org/projects/scapy/builds/

This happens in weird cases where dependencies don't match between readthedocs and our github tests.

Just bumped the dependencies, it should get back to working.

@polybassa
Copy link
Contributor Author

ok, great.. so we can merge this one?

@polybassa
Copy link
Contributor Author

Ready for merge

scapy/sendrecv.py Outdated Show resolved Hide resolved
@gpotter2
Copy link
Member

gpotter2 commented Oct 23, 2023

Okay there are many issues with this PR.
Generally help(srp) is unreadable, and the variables don't have the same name, nor are in the same order in the function and in the docstring.

For readers, this is what help(srp) looks like:

Help on function srp in module scapy.sendrecv:

srp(x, promisc=None, iface=None, iface_hint=None, filter=None, nofilter=0, type=3, *args, **kargs)
    Send and receive packets at layer 2

    :param pks: SuperSocket instance to send/receive packets
    :type pkt: SuperSocket

    :param pkt: Packet or iterable of packets to be sent.
    :type pkt: _PacketIterable
    :param timeout: How much time to wait after the last packet
                    has been sent. Defaults to None.
    :type timeout: Optional[int]
    :param inter: Delay between two packets during sending. Defaults to 0.
    :type inter: Optional[int]

    :param verbose: Set verbosity level. Defaults to None.
    :type verbose: Optional[int]

    :param chainCC: If True, KeyboardInterrupts will be forwarded.
                    Defaults to False.
    :type chainCC: Optional[bool]

    :param retry: If positive, how many times to resend unanswered packets.
                  If negative, how many times to retry when no more packets
                  are answered. Defaults to 0.
    :type retry: Optional[int]
  • arguments are not in order
  • don't have the correct names
  • and the typing information makes it unreadable

@polybassa
Copy link
Contributor Author

I totally agree, srp and srp1 is broken. Sorry. I'll fix it

@polybassa
Copy link
Contributor Author

@gpotter2 Cleaned it up.

@polybassa
Copy link
Contributor Author

Could you please review?

@polybassa
Copy link
Contributor Author

@gpotter2 I've resolved the conflicts on this PR, could you please have a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants