Skip to content

bpo-43422: Revert _decimal C API addition#24792

Closed
pitrou wants to merge 1 commit into
python:masterfrom
skrah:revert_decimal_capsule_api
Closed

bpo-43422: Revert _decimal C API addition#24792
pitrou wants to merge 1 commit into
python:masterfrom
skrah:revert_decimal_capsule_api

Conversation

@pitrou

@pitrou pitrou commented Mar 8, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Stefan Krah requested the reversal of these (unreleased) changes, quoting him:

The capsule API does not meet my testing standards, since I've focused
on the upstream mpdecimal in the last couple of months.
Additionally, I'd like to refine the API, perhaps together with the
Arrow community.

https://bugs.python.org/issue43422

@pitrou pitrou requested a review from rhettinger as a code owner March 8, 2021 22:52
@pitrou pitrou requested a review from mdickinson March 8, 2021 22:52
@erlend-aasland

erlend-aasland commented Mar 9, 2021

Copy link
Copy Markdown
Contributor

Shouldn't this require a NEWS item? #21519 had one, and the API has been present in all the 3.10 alphas released so far.

mdickinson
mdickinson approved these changes Mar 21, 2021
View reviewed changes

@mdickinson mdickinson left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. A couple of questions, but nothing show-stopping.

Comment thread Modules/_decimal/_decimal.c
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2008-2020 Stefan Krah. All rights reserved.
* Copyright (c) 2008-2012 Stefan Krah. All rights reserved.

@mdickinson mdickinson Mar 21, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change seems odd; is this definitely correct? (Should "2012" have been "2021" here?)

I don't think it's terribly consequential, and I'm happy for the change to stay if that keeps us compatible with upstream.

Comment thread Modules/_decimal/tests/deccheck.py
import _pydecimal as P


C = import_fresh_module('decimal', fresh=['_decimal'])

@mdickinson mdickinson Mar 21, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are these changes related to the purpose of the PR, or just incidental?

@pitrou

pitrou commented Mar 21, 2021

Copy link
Copy Markdown
Member Author

For the record, this PR is straight from Stefan's own branch here: master...skrah:revert_decimal_capsule_api
I'm reluctant to do any changes that aren't necessary for inclusion.

@pitrou

pitrou commented Mar 21, 2021

Copy link
Copy Markdown
Member Author

I'll add a NEWS item soon.

@pitrou pitrou mentioned this pull request Mar 21, 2021
Merged
@pitrou

pitrou commented Mar 21, 2021

Copy link
Copy Markdown
Member Author

Since this PR was created from Stefan's fork, I've opened #24960 from my fork in order to add a NEWS item. Closing this one.

@pitrou pitrou closed this Mar 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhettinger rhettinger Awaiting requested review from rhettinger

1 more reviewer

@mdickinson mdickinson mdickinson approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

awaiting merge skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@pitrou @erlend-aasland @mdickinson @the-knights-who-say-ni @bedevere-bot @skrah