Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
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 to0ce377aComparevstinner commentedOct 28, 2023
I would prefer to make them public if they are useful. |
skirpichev commentedOct 28, 2023
@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 to2c6002fCompareUh oh!
There was an error while loading.Please reload this page.
vstinner left a comment
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 |
vstinner commentedOct 30, 2023
If you want to document these constants, make them public. |
ngoldbaum commentedOct 30, 2023
It looks like numpy uses |
skirpichev commentedOct 31, 2023
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
|
skirpichev commentedNov 3, 2023
I have made the requested changes; please review again |
skirpichev commentedNov 3, 2023
I didn't expect the Spanish Inquisition |
skirpichev commentedNov 3, 2023
vstinner commentedNov 4, 2023
skirpichev commentedNov 5, 2023
Hmm, no, it seems we really want this, e.g.:gmpy2/gmpy2#452 (see first removed commit). Then maybe we should expose this in sys.hash_info struct too? |
vstinner left a comment
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."
skirpichev commentedDec 15, 2023
Perhaps, I should use |
skirpichev commentedDec 21, 2023
skirpichev commentedMar 4, 2024
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>
vstinner left a comment
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.
mdickinson left a comment
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.
vstinner commentedMar 9, 2024
Merged, thank you@skirpichev. |
skirpichev commentedMar 10, 2024
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.