GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||
GeezerVison is showing github.com — gh-111389: expose _PyHASH_INF/BITS/MODULUS/IMAG macros as public by skirpichev · Pull Request #111418 · python/cpython Conversation
|
|
I would prefer to make them public if they are useful. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
@vstinner, are you about documenting these macroses somewhere (I propose https://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)? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
1f9f2fc to
2c6002f
Compare
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Please don't document private APIs.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
If you want to document these constants, make them public. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
It looks like numpy uses |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I have made the requested changes; please review again |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I didn't expect the Spanish Inquisition |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…etY.rst Co-authored-by: Victor Stinner <[email protected]>
Was rendered: "See also the PyTypeObject.tp_hash member and Hashing of numeric types for more details about hashing of numeric types."
|
Perhaps, I should use |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I have made the requested changes; please review again |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks for making the requested changes! @vstinner: please review the changes made to this pull request. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Victor Stinner <[email protected]>
There was a problem 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?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Merged, thank you @skirpichev. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks to all for review and patience. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…python#111418) Co-authored-by: Victor Stinner <[email protected]>
…python#111418) Co-authored-by: Victor Stinner <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.