FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-95389: socketmodule: expose popular `ETHERTYPE_*` constants by noamcohen97 · Pull Request #95390 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

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

Merged
erlend-aasland merged 19 commits into
python:mainfrom
noamcohen97:raw-socket
Nov 7, 2022
ConversationCommits (19)ChecksFiles changed
Merged

gh-95389: socketmodule: expose popular ETHERTYPE_* constants#95390
erlend-aasland merged 19 commits into
python:mainfrom
noamcohen97:raw-socket

Conversation

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

Copy link
Copy Markdown
Contributor

noamcohen97 commented Jul 28, 2022

Copy link
Copy Markdown
Contributor Author

still missing docs, news and tests

noamcohen97 marked this pull request as ready for review July 30, 2022 14:23
erlend-aasland reviewed Jul 30, 2022
View reviewed changes
Comment thread Modules/socketmodule.c Outdated
erlend-aasland reviewed Jul 30, 2022
View reviewed changes
Comment thread Doc/library/socket.rst
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 requested a review from tiran August 1, 2022 10:46
tiran requested changes Aug 1, 2022
View reviewed changes

tiran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Comment thread Doc/library/socket.rst
Comment thread Doc/library/socket.rst Outdated
Comment thread Modules/socketmodule.c Outdated

bedevere-bot commented Aug 1, 2022

Copy link
Copy Markdown

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 commented Aug 1, 2022

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

bedevere-bot commented Aug 1, 2022

Copy link
Copy Markdown

Thanks for making the requested changes!

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

bedevere-bot requested a review from tiran August 1, 2022 12:50

tiran commented Aug 1, 2022

Copy link
Copy Markdown
Member

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 commented Aug 1, 2022

Copy link
Copy Markdown
Contributor Author

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

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

CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Some documentation suggestions and a couple comments.

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

CAM-Gerlach left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Some additional docs followups

Comment thread Doc/library/socket.rst Outdated
Comment thread Doc/library/socket.rst
Comment thread Doc/library/socket.rst
Comment thread Doc/library/socket.rst Outdated
noamcohen97 and others added 2 commits August 2, 2022 07:07
Apply suggestions from code review
43b5d45 
Co-authored-by: CAM Gerlach <CAM.Gerlach@Gerlach.CAM>
doc fix
fc9bc01

noamcohen97 commented Sep 7, 2022

Copy link
Copy Markdown
Contributor Author

@tiran ping

noamcohen97 commented Oct 14, 2022

Copy link
Copy Markdown
Contributor Author

Any updates on this?

CAM-Gerlach commented Oct 15, 2022

Copy link
Copy Markdown
Member

I pinged people for another review here

erlend-aasland self-assigned this Oct 17, 2022

erlend-aasland commented Nov 7, 2022

Copy link
Copy Markdown
Contributor

AFAICS, Christian's remarks have been addressed.

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

erlend-aasland merged commit 80c08d1 into python:main Nov 7, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

CAM-Gerlach CAM-Gerlach left review comments
erlend-aasland erlend-aasland approved these changes
tiran Awaiting requested review from tiran

Assignees

erlend-aasland

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL