Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bpo-43422: Revert _decimal C API addition #24792

Closed
wants to merge 1 commit into from
Closed

bpo-43422: Revert _decimal C API addition #24792

wants to merge 1 commit into from

Conversation

pitrou
Copy link
Member

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

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
Copy link
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
Copy link
Member

@mdickinson mdickinson left a comment

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.

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.
Copy link
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.

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


C = import_fresh_module('decimal', fresh=['_decimal'])
Copy link
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
Copy link
Member Author

pitrou commented Mar 21, 2021

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
Copy link
Member Author

pitrou commented Mar 21, 2021

I'll add a NEWS item soon.

@pitrou pitrou mentioned this pull request Mar 21, 2021
Merged
@pitrou
Copy link
Member Author

pitrou commented Mar 21, 2021

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

@mdickinson mdickinson mdickinson approved these changes

@rhettinger rhettinger Awaiting requested review from rhettinger

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