Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-111389: expose _PyHASH_INF/BITS/MODULUS/IMAG macros as public#111418
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…publicThis partially revertspython#107026.
1edec34
to0ce377a
CompareI would prefer to make them public if they are useful. |
@vstinner, are you about documenting these macroses somewhere (I proposehttps://docs.python.org/3/library/sys.html#sys.hash_info, where we could mention them in descriptions for different fields) AND about renaming them too (add PyUnstable_ aliases)? |
1f9f2fc
to2c6002f
CompareUh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Please don't document private APIs.
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 |
If you want to document these constants, make them public. |
It looks like numpy uses |
Ok, I've added PyUnstable prefixes. If that's ok, I'll add a news entry. The other option is to revert to 0ce377a. I don't think we could just remove underscores, given the I have made the requested changes; please review again BTW, maybe we shouldn't export
|
I have made the requested changes; please review again |
I didn't expect the Spanish Inquisition |
Hmm, no, it seems we really want this, e.g.:aleaxit/gmpy#452 (see first removed commit). Then maybe we should expose this in sys.hash_info struct too? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm working on PR#112095 which adds an "infrastructure" to test the PyHash API. I suggest waiting until this PR is merged before considering exposing these constants to the public C API.
Uh oh!
There was an error while loading.Please reload this page.
Misc/NEWS.d/next/C API/2023-11-15-09-24-51.gh-issue-111418.FYYetY.rst OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
…etY.rstCo-authored-by: Victor Stinner <vstinner@python.org>
Was rendered: "See also the PyTypeObject.tp_hash member and Hashingof numeric types for more details about hashing of numeric types."
Perhaps, I should use |
I have made the requested changes; please review again |
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Victor Stinner <vstinner@python.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM, I just suggest to add a comment.
@mdickinson: Would you mind to double check this PR?
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks fine to me.
Merged, thank you@skirpichev. |
Thanks to all for review and patience. |
…python#111418)Co-authored-by: Victor Stinner <vstinner@python.org>
…python#111418)Co-authored-by: Victor Stinner <vstinner@python.org>
Uh oh!
There was an error while loading.Please reload this page.