Skip to content

Conversation

StanFromIreland
Copy link
Member

@StanFromIreland StanFromIreland commented Oct 18, 2025

@StanFromIreland StanFromIreland marked this pull request as ready for review October 18, 2025 15:17
@vstinner
Copy link
Member

cc @skirpichev @picnixz

Co-authored-by: Victor Stinner <[email protected]>
@merwok
Copy link
Member

merwok commented Oct 19, 2025

(Please edit the commit message when merging so that we’re not tagged forever by pushes in clones! thanks 🙂)

@StanFromIreland
Copy link
Member Author

StanFromIreland commented Oct 19, 2025

(Please edit the commit message when merging so that we’re not tagged forever by pushes in clones! thanks 🙂)

Éric, all of my commits up to that point had the commit message "Commit"? I tagged you in the PR description, I don't think that should be a burden for longer than this PR is open? There is an "unsubscribe" button in the right sidebar.

@skirpichev skirpichev requested a review from vstinner October 19, 2025 08:53
Copy link
Contributor

@skirpichev skirpichev 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, modulo one nitpick about decimal.rst.

I'm not sure also about naming for the constant. But lets see what other people think.


.. versionadded:: 3.8.3

.. data:: SPEC_VERSION
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think that this should be moved up before "The constants in this section are only relevant for the C module." sentence of the first paragraph. Which should be also rephrased, i.e. "Following constants in <...>". Clearly, spec version is relevant for both modules.

BTW, I'm not sure about naming. Maybe more verbose "DECIMAL_SPEC_VERSION"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I'm not sure about naming. Maybe more verbose "DECIMAL_SPEC_VERSION"?

I have a similar sentiment, I simply used this since it was in the discussion.

@skirpichev
Copy link
Contributor

all of my commits up to that point had the commit message "Commit"? I tagged you in the PR description

It seems core devs sometimes use PR descriptions for commit messages. So, it's rather a reminder for one, who will merge this pr.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure also about naming for the constant. But lets see what other people think.

decimal.SPEC_VERSION is fine by me. Skip also suggested it at #76007 (comment).

Co-authored-by: Hugo van Kemenade <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment