Skip to content

bpo-37822: Add math.as_integer_ratio(). #15210

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
Closed
serhiy-storchaka wants to merge 2 commits into from
Closed

bpo-37822: Add math.as_integer_ratio(). #15210

serhiy-storchaka wants to merge 2 commits into from

Conversation

@serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Aug 11, 2019 •
edited by bedevere-bot
Loading

@serhiy-storchaka serhiy-storchaka added the type-feature A feature request or enhancement label Aug 11, 2019
@jdemeyer
Copy link
Contributor

jdemeyer commented Aug 12, 2019

I think it would be good to define more precisely what are the output types. We require the output of as_integer_ratio to be two integers, but what does "integer" mean in this context? A Python int? An object which implements __index__? An instance of numbers.Integral?

My personal vote would be to require __index__ (I don't mean that we should check that condition, but we should document it).

@jdemeyer jdemeyer mentioned this pull request Aug 19, 2019
Closed
brandtbucher
brandtbucher requested changes Aug 23, 2019
View reviewed changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

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

One small issue I noticed:

Modules/mathmodule.c
math.as_integer_ratio
x: object
/
greatest common divisor of x and y
Copy link
Member

@brandtbucher brandtbucher Aug 23, 2019

Choose a reason for hiding this comment

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

This looks like a copy-paste error. Maybe:

Suggested change
greatest common divisor of x and y
Return the rational representation of x as the pair (numerator, denominator).

Copy link
Member Author

@serhiy-storchaka serhiy-storchaka Aug 24, 2019

Choose a reason for hiding this comment

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

Thank you. But this proposition was rejected.

@serhiy-storchaka serhiy-storchaka deleted the math-as_integer_ratio branch August 24, 2019 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandtbucher brandtbucher brandtbucher requested changes

@mdickinson mdickinson Awaiting requested review from mdickinson

@rhettinger rhettinger Awaiting requested review from rhettinger

Assignees

No one assigned

Labels

awaiting core review type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@serhiy-storchaka @jdemeyer @brandtbucher @the-knights-who-say-ni @bedevere-bot