Skip to content

gh-125350: Wrap unused argument in timemodule.c with Py_UNUSED #125351

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 2 commits into from
Closed

gh-125350: Wrap unused argument in timemodule.c with Py_UNUSED #125351

wants to merge 2 commits into from

Conversation

rruuaanng
Copy link
Contributor

@rruuaanng rruuaanng commented Oct 12, 2024 •
edited by bedevere-app bot
Loading

@bedevere-app
Copy link

bedevere-app bot commented Oct 12, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@bedevere-app bedevere-app bot mentioned this pull request Oct 12, 2024
Closed
@rruuaanng rruuaanng marked this pull request as ready for review October 12, 2024 08:03
picnixz
picnixz reviewed Oct 12, 2024
View reviewed changes
@bedevere-app
Copy link

bedevere-app bot commented Oct 12, 2024

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

picnixz
picnixz approved these changes Oct 12, 2024
View reviewed changes
Copy link
Member

@picnixz picnixz 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! Hopefully this one won't count as a cosmetic change.

@rruuaanng
Copy link
Contributor Author

Maybe not, It solves the warning.

@erlend-aasland
Copy link
Contributor

IMO, it's fine to mark unused params with Py_UNUSED. I'll wait a couple of days to give @pganssle a chance to chime in before landing this, though.

@rruuaanng

This comment was marked as off-topic.

Sign in to view
@erlend-aasland erlend-aasland self-assigned this Oct 15, 2024
@erlend-aasland erlend-aasland mentioned this pull request Oct 15, 2024
Closed
@erlend-aasland
Copy link
Contributor

I'm terribly sorry, but I'm changing my mind regarding this change :( This is purely a cosmetic change, and by merging it, we would send a signal that we encourage similar change. This is a signal we don't want to send, so I'm rejecting the change and closing the PR and the linked issue.

Thanks for the PR, and thanks for the report, but we'll pass on this.

@rruuaanng rruuaanng deleted the gh125350 branch October 15, 2024 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@picnixz picnixz picnixz approved these changes

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

@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner

@abalkin abalkin Awaiting requested review from abalkin abalkin is a code owner

Assignees

@erlend-aasland erlend-aasland

Labels

awaiting merge skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rruuaanng @erlend-aasland @picnixz