Skip to content

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

Closed
devnexen wants to merge 4 commits into
python:mainfrom
devnexen:netbsd_can_flags
Closed

bpo-45172: netbsd socketmodule CAN protocol constants addition.#28288
devnexen wants to merge 4 commits into
python:mainfrom
devnexen:netbsd_can_flags

Conversation

@devnexen

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

Copy link
Copy Markdown
Contributor

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

github-actionsBot commented Oct 12, 2021

Copy link
Copy Markdown

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

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Oct 12, 2021
@FFY00

FFY00 commented Oct 15, 2021

Copy link
Copy Markdown
Member

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

vstinner
vstinner reviewed Oct 15, 2021
View reviewed changes
Comment threadModules/socketmodule.c Outdated
Comment on lines 7710 to 7760

@vstinnervstinnerOct 15, 2021

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.

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

@devnexen

devnexen commented Oct 15, 2021

Copy link
Copy Markdown
ContributorAuthor

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

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

bedevere-bot commented Oct 21, 2021

Copy link
Copy Markdown

🤖 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-botbedevere-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

@erlend-aaslanderlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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?

Comment threadconfigure.ac Outdated
erlend-aasland
erlend-aasland reviewed May 16, 2022
View reviewed changes
Comment threadpyconfig.h.in Outdated
@erlend-aasland

erlend-aasland commented May 16, 2022

Copy link
Copy Markdown
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

@erlend-aaslanderlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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

@vstinnervstinner 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.

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

Comment threadMisc/NEWS.d/next/Library/2021-09-11-13-45-47.bpo-45172.T0LUgT.rst Outdated
@erlend-aasland

erlend-aasland commented May 17, 2022

Copy link
Copy Markdown
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

devnexen commented May 17, 2022

Copy link
Copy Markdown
ContributorAuthor

I updated the entry accordingly.

@erlend-aasland

erlend-aasland commented May 18, 2022

Copy link
Copy Markdown
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

devnexen commented May 18, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@erlend-aasland

erlend-aasland commented May 18, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Nonetheless, thanks for your interest in improving CPython :)

@devnexendevnexenmannequin mentioned this pull request May 18, 2022
Closed
@vstinner

vstinner commented May 18, 2022

Copy link
Copy Markdown
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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erlend-aaslanderlend-aaslanderlend-aasland approved these changes
@vstinnervstinnervstinner approved these changes

Assignees

No one assigned

Labels

awaiting mergetype-featureA 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