Skip to content

bpo-45172: netbsd socketmodule CAN protocol constants addition. #28288

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
Closed
wants to merge 4 commits into from
Closed

bpo-45172: netbsd socketmodule CAN protocol constants addition. #28288

wants to merge 4 commits into from

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented Sep 11, 2021 •
edited by bedevere-bot
Loading

@devnexen devnexen changed the title netbsd socketmodule CAN protocol constants addition. bpo-45172: netbsd socketmodule CAN protocol constants addition. Sep 11, 2021
@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Sep 11, 2021
@github-actions
Copy link

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale Stale PR or inactive for long period of time. label Oct 12, 2021
@FFY00
Copy link
Member

FFY00 commented Oct 15, 2021

This looks alright. Perhaps a test would be good. @vstinner could you have a look?

vstinner
vstinner reviewed Oct 15, 2021
View reviewed changes
Modules/socketmodule.c Outdated
Comment on lines 7710 to 7760
#if defined(HAVE_LINUX_CAN_RAW_H)
PyModule_AddIntMacro(m, CAN_RAW_ERR_FILTER);
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move CAN_RAW_ERR_FILTER above in a separated block to avoid nested #if.

@devnexen
Copy link
Contributor Author

well tests are based on the presence of constants existence and netbsd share the same naming.

@github-actions github-actions bot removed the stale Stale PR or inactive for long period of time. label Oct 16, 2021
@ambv ambv added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 21, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @ambv for commit 799d8e13b2653916ced73ee9855ed2a61c841117 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 21, 2021
erlend-aasland
erlend-aasland reviewed May 16, 2022
View reviewed changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

@devnexen, can you resolve conflicts and address Victor's review?

@devnexen devnexen force-pushed the netbsd_can_flags branch from 799d8e1 to cfff139 Compare May 16, 2022 20:29
@devnexen devnexen force-pushed the netbsd_can_flags branch from cfff139 to 7113d49 Compare May 16, 2022 20:39
erlend-aasland
erlend-aasland reviewed May 16, 2022
View reviewed changes
@erlend-aasland
Copy link
Contributor

Please avoid force-pushing PRs. I'm now unable to see the changes between your first version of the PR and the new version. It makes the reviewers job easier if you use git fetch --all && git merge --no-ff main.

erlend-aasland
erlend-aasland approved these changes May 16, 2022
View reviewed changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Looks good to me. If Victor gives a thumbs up, I'll merge in a day or two.

vstinner
vstinner approved these changes May 17, 2022
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM. @erlend-aasland: you can merge it if you're fine with the current NEWS entry.

@erlend-aasland
Copy link
Contributor

LGTM. @erlend-aasland: you can merge it if you're fine with the current NEWS entry.

Thanks, I'll merge after the NEWS entry has been amended.

@devnexen
Copy link
Contributor Author

I updated the entry accordingly.

@erlend-aasland
Copy link
Contributor

It looks like gh-30066 already added support for the CAN_RAW_ERR_FILTER flag. Please take a look at that PR; it was merged a couple of months ago.

@devnexen
Copy link
Contributor Author

Ah I did not notice that one, well if this is supported already then my PR is pointless now.

@devnexen devnexen closed this May 18, 2022
@erlend-aasland
Copy link
Contributor

erlend-aasland commented May 18, 2022 •
edited
Loading

Nonetheless, thanks for your interest in improving CPython :)

@devnexen devnexen mannequin mentioned this pull request May 18, 2022
Closed
@vstinner
Copy link
Member

Well, the important thing is that Python got CAN support on NetBSD ;-) 40fcd16 is part of Python 3.11: you can test the beta1!

Yeah, thanks for your interest in enhancing Python!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@vstinner vstinner vstinner approved these changes

Assignees
No one assigned
Labels
awaiting merge type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@devnexen @FFY00 @bedevere-bot @erlend-aasland @vstinner @ambv @serhiy-storchaka @the-knights-who-say-ni