Skip to content

Use _PyLong_GetOne() in long_pow() - #125474

Closed
hikariyo wants to merge 2 commits into
python:mainfrom
hikariyo:reduce-null-check
Closed

hikariyo wants to merge 2 commits into
python:mainfrom
hikariyo:reduce-null-check

Conversation

@hikariyo

Copy link
Copy Markdown

Just another place like what #125044 did. It makes code clearer.

@bedevere-app

bedevere-app Bot commented Oct 14, 2024

Copy link
Copy Markdown

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.

@nineteendo

Copy link
Copy Markdown
Contributor

Can you update the branch?

@tomasr8 tomasr8 changed the title Complete what #125044 did Use _PyLong_GetOne() in long_pow() Oct 14, 2024
@bedevere-app

bedevere-app Bot commented Oct 15, 2024

Copy link
Copy Markdown

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.

@rruuaanng

Copy link
Copy Markdown
Contributor

Please add title label, like this

gh-<issue_id>: xxx

@hikariyo

Copy link
Copy Markdown
Author

Please add title label, like this

gh-<issue_id>: xxx

This is a trivial change which might skip issue 😊

@rruuaanng

Copy link
Copy Markdown
Contributor

And, this has been rejected by the core-dev, I don't think this modification is meaningful.

@hikariyo

hikariyo commented Oct 15, 2024 •

Copy link
Copy Markdown
Author

And, this has been rejected by the core-dev, I don't think this modification is meaningful.

But similar change has been merged as I mentioned, and I think the last PR has been rejected because it changed so many places which makes code ugly, different from this PR.

I think it is meaningful to keep the code style the same in the same file.

@rruuaanng

Copy link
Copy Markdown
Contributor

My suggestion is to open an issue and listen to other people's opinions.

@hikariyo

Copy link
Copy Markdown
Author

My suggestion is to open an issue and listen to other people's opinions.

Since a core-dev has approved similar change, and I'm just doing something to finish his incomplete change, it should be just fine.

I found this place when studying this file and want to do something to make the code style the same 🥲

@rruuaanng

Copy link
Copy Markdown
Contributor

You might be talking about this
https://github.com/python/cpython/pull/125044/files

In fact, changing the code style alone isn't allowed. You should also modify other functions to pass.

@hikariyo

Copy link
Copy Markdown
Author

Okay, sorry to trouble you 🙏.

@hikariyo hikariyo closed this Oct 15, 2024
@erlend-aasland

Copy link
Copy Markdown
Contributor

And, this has been rejected by the core-dev, I don't think this modification is meaningful.

@rruuaanng, please try and keep a polite tone.

@hikariyo: Thanks for the PR, but as has been noted, pretty much all code changes, including refactorings1, need issues. Please see the devguide for more info :)

Footnotes

  1. this PR counts as refactoring ↩

@rruuaanng

Copy link
Copy Markdown
Contributor

And, this has been rejected by the core-dev, I don't think this modification is meaningful.

@rruuaanng, please try and keep a polite tone.

@hikariyo: Thanks for the PR, but as has been noted, pretty much all code changes, including refactorings1, need issues. Please see the devguide for more info :)

I'm sorry, I'm not hostile. Maybe I should bring a smile🙂

Footnotes

  1. this PR counts as refactoring ↩

@erlend-aasland

Copy link
Copy Markdown
Contributor

I'm sorry, I'm not hostile. Maybe I should bring a smile

It often helps to use more friendly words. Instead of "I don't think this modification is meaningful", try instead to ask what the author intends to achieve with the change. @rruuaanng, see also https://opensource.guide/how-to-contribute/#how-to-submit-a-contribution.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants