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.

Sign up for GitHub

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
Loading

@noamcohen97
Copy link
Contributor Author

still missing docs, news and tests

@noamcohen97 noamcohen97 marked this pull request as ready for review July 30, 2022 14:23
@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 <[email protected]>
tiran
tiran requested changes Aug 1, 2022
View reviewed changes
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

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

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

I have made the requested changes; please review again.

@bedevere-bot
Copy link

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 August 1, 2022 12:50
@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

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 reviewed changes
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

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
CAM-Gerlach
CAM-Gerlach reviewed Aug 2, 2022
View reviewed changes
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@tiran ping

@noamcohen97
Copy link
Contributor Author

Any updates on this?

@CAM-Gerlach
Copy link
Member

I pinged people for another review here

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

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CAM-Gerlach CAM-Gerlach CAM-Gerlach left review comments

@erlend-aasland erlend-aasland erlend-aasland approved these changes

@tiran tiran Awaiting requested review from 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.
5 participants
@noamcohen97 @bedevere-bot @tiran @CAM-Gerlach @erlend-aasland