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

Jump to bottom

gh-95389: socketmodule: expose popular ETHERTYPE_* constants #95390

Merged
merged 19 commits into from Nov 7, 2022
Merged

gh-95389: socketmodule: expose popular ETHERTYPE_* constants #95390

merged 19 commits into from Nov 7, 2022

Conversation

noamcohen97
Copy link
Contributor

@noamcohen97 noamcohen97 commented Jul 28, 2022 •
edited by bedevere-bot

@noamcohen97
Copy link
Contributor Author

noamcohen97 commented Jul 28, 2022

still missing docs, news and tests

@noamcohen97 noamcohen97 marked this pull request as ready for review Jul 30, 2022
erlend-aasland
erlend-aasland reviewed Jul 30, 2022
View changes
Modules/socketmodule.c Outdated Show resolved Hide resolved
@noamcohen97 @erlend-aasland
Update Misc/NEWS.d/next/Core and Builtins/2022-07-30-14-10-27.gh-issu…
18b0286 
…e-95389.nSGEkG.rst

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@protonmail.com>
@erlend-aasland erlend-aasland requested a review from tiran Aug 1, 2022
tiran
tiran requested changes Aug 1, 2022
View changes
Copy link
Member

@tiran tiran left a comment

Your patch does not add any ETH_P constants for me. I guess you need a configure check and include for a public header file. I'm not sure which header file is the best, perhaps net/ethernet.h or netinet/ether.h?

Doc/library/socket.rst Show resolved Hide resolved
Doc/library/socket.rst Outdated Show resolved Hide resolved
Modules/socketmodule.c Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

bedevere-bot commented Aug 1, 2022

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@noamcohen97
Copy link
Contributor Author

noamcohen97 commented Aug 1, 2022

I have made the requested changes; please review again.

@bedevere-bot
Copy link

bedevere-bot commented Aug 1, 2022

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

@bedevere-bot bedevere-bot requested a review from tiran Aug 1, 2022
@tiran
Copy link
Member

tiran commented Aug 1, 2022

FreeBSD seems to have the header file net/ethernet.h, too. The BSD variant uses different constant names like ETHERTYPE_IP. These are also supported by Linux's net/ethernet.h. Could you please check other platforms and use a common subset for the constants?

I know it's a PITB, but Python supports macOS and FreeBSD, too. Any new feature should work on all platforms if possible.

@noamcohen97
Copy link
Contributor Author

noamcohen97 commented Aug 1, 2022

Nice! macOS support them as well :)
Thanks for pointing it out!

@noamcohen97 noamcohen97 changed the title gh-95389: socketmodule: expose popular ETH_P_* constants gh-95389: socketmodule: expose popular ETHERTYPE_* constants Aug 1, 2022
CAM-Gerlach
CAM-Gerlach reviewed Aug 1, 2022
View changes
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Some documentation suggestions and a couple comments.

Doc/library/socket.rst Outdated Show resolved Hide resolved
Doc/library/socket.rst Outdated Show resolved Hide resolved
Doc/library/socket.rst Outdated Show resolved Hide resolved
Doc/library/socket.rst Show resolved Hide resolved
Doc/library/socket.rst Outdated Show resolved Hide resolved
Doc/library/socket.rst Outdated Show resolved Hide resolved
noamcohen97 and others added 3 commits Aug 1, 2022
@noamcohen97 @CAM-Gerlach
Update Doc/library/socket.rst
7fd3467 
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@noamcohen97 @CAM-Gerlach
Apply suggestions from code review
257e46b 
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@noamcohen97
added footnote
36b1d6a
CAM-Gerlach
CAM-Gerlach reviewed Aug 2, 2022
View changes
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Some additional docs followups

Doc/library/socket.rst Outdated Show resolved Hide resolved
Doc/library/socket.rst Show resolved Hide resolved
Doc/library/socket.rst Show resolved Hide resolved
Doc/library/socket.rst Outdated Show resolved Hide resolved
noamcohen97 and others added 2 commits Aug 2, 2022
@noamcohen97 @CAM-Gerlach
Apply suggestions from code review
43b5d45 
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
@noamcohen97
doc fix
fc9bc01
@noamcohen97
Copy link
Contributor Author

noamcohen97 commented Sep 7, 2022

@tiran ping

@noamcohen97
Copy link
Contributor Author

noamcohen97 commented Oct 14, 2022

Any updates on this?

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Oct 15, 2022

I pinged people for another review here

@erlend-aasland erlend-aasland self-assigned this Oct 17, 2022
@erlend-aasland
Copy link
Contributor

erlend-aasland commented Nov 7, 2022

AFAICS, Christian's remarks have been addressed.

Thanks for the issue, PR, and your patience, Noam 🙂

@erlend-aasland erlend-aasland merged commit 80c08d1 into python:main Nov 7, 2022
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CAM-Gerlach CAM-Gerlach

@erlend-aasland erlend-aasland

@tiran tiran

Assignees

@erlend-aasland erlend-aasland

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@noamcohen97 @bedevere-bot @tiran @CAM-Gerlach @erlend-aasland