[proxy] web.archive.org← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light
/ cpython Public
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-33083 - math.factorial accepts non-integral Decimal instances #6149

Merged
merged 6 commits into from Sep 3, 2018

Conversation

Copy link
Member

pablogsal commented Mar 19, 2018

pablogsal changed the title bpo33083 - math.factorial accepts non-integral Decimal instances bpo-33083 - math.factorial accepts non-integral Decimal instances Mar 19, 2018
@@ -0,0 +1,3 @@
math.factorial no longer accept arguments that are not int-like. A

Choose a reason for hiding this comment

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

Sorry for nit-picking a bit. But

"math.factorial no longer accepts arguments.." (accept -> accepts).

emmited -> emitted

Copy link
Member Author

pablogsal Jul 30, 2018

Choose a reason for hiding this comment

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

Thanks! :)

Copy link
Contributor

taleinat left a comment

It was decided on the issue to avoid deprecating floats as part of this issue. Please undo the changes regarding deprecation of floats; though perhaps keep them stashed somewhere for future use...

}
else{
pyint_form = PyNumber_Index(arg);
if( pyint_form == NULL){
Copy link
Contributor

taleinat Jul 30, 2018

Choose a reason for hiding this comment

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

Fix the spacing on this and the next line.

Copy link

bedevere-bot commented Jul 30, 2018

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Copy link
Member Author

pablogsal commented Jul 30, 2018

I have made the requested changes; please review again

Copy link

bedevere-bot commented Jul 30, 2018

Thanks for making the requested changes!

@taleinat: please review the changes made to this pull request.

Copy link
Contributor

taleinat left a comment

The NEWS entry no longer correctly reflects the changes made; please revise.

Copy link

bedevere-bot commented Jul 30, 2018

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be poked with soft cushions!

Copy link
Member Author

pablogsal commented Jul 30, 2018

I have made the requested changes; please review again. Thank you for the whitespace fix!

Copy link

bedevere-bot commented Jul 30, 2018

Thanks for making the requested changes!

@taleinat: please review the changes made to this pull request.

pablogsal force-pushed the bpo33083 branch 2 times, most recently from a449b46 to 4b465a4 Compare Jul 30, 2018
Copy link
Contributor

taleinat left a comment

LGTM

Copy link
Member

mdickinson left a comment

LGTM

Copy link
Member

vstinner left a comment

LGTM, you can merge it @pablogsal ;-)

Copy link
Member

vstinner left a comment

LGTM. @pablogsal you can merge it.

Copy link
Member

vstinner left a comment

LGTM.

pablogsal merged commit e9ba370 into python:master Sep 3, 2018
pablogsal deleted the bpo33083 branch Sep 3, 2018
yahya-abou-imran pushed a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018
pythonGH-6149)

math.factorial() was accepting non-integral Decimal instances. This is inconsistent with the actual behaviour for floats, which are not accepted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants