◐ Shell
reader mode source ↗
Skip to content

bpo-37819: Add Fraction.as_integer_ratio()#15212

Merged
rhettinger merged 3 commits into
python:masterfrom
rhettinger:fraction_ratio
Aug 11, 2019
Merged

bpo-37819: Add Fraction.as_integer_ratio()#15212
rhettinger merged 3 commits into
python:masterfrom
rhettinger:fraction_ratio

Conversation

@rhettinger

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

Copy link
Copy Markdown
Contributor

@rhettinger rhettinger added type-feature A feature request or enhancement needs backport to 3.8 labels Aug 11, 2019
gvanrossum
gvanrossum reviewed Aug 11, 2019
View reviewed changes

@gvanrossum gvanrossum 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.

Hide comment

I would change "exactly equal" to just "equal" -- there's nothing to be gained by adding "exactly".

aeros
aeros reviewed Aug 11, 2019
View reviewed changes

@aeros aeros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

Thanks for the PR @rhettinger.

I had some minor suggestions:

@aeros

aeros commented Aug 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

I didn't include this in my review since it's not directly related to the PR, but I noticed some minor inconsistencies in the tests between the different versions of *.as_integer_ratio(). For example, the test function in float was called test_floatasratio() instead of test_as_integer_ratio() (which is used by decimal.py and now in this PR), and it also seemed to be missing the test that decimal uses to ensure a ValueError is raised when attempting to use an invalid string:

       self.assertRaises(ValueError,
                          Decimal.as_integer_ratio, Decimal('snan123'))

Also, as far as I can tell, there's no test coverage at all for the int version of the method. Should I open a new issue for this?

@rhettinger
Address reviewer comments
b521c42
@rhettinger
Minor word-smithing. …
b55d9d8 
* The other methods spell fraction with a capital F.
* The word "original" doesn't make sense because the original
  fraction is reduced:
     Fraction(4, 6).as_integer_ratio() |-> (2, 3)
@rhettinger rhettinger merged commit f03b4c8 into python:master Aug 11, 2019
@rhettinger rhettinger deleted the fraction_ratio branch August 11, 2019 21:41
@miss-islington

miss-islington commented Aug 11, 2019

Copy link
Copy Markdown
Contributor

Thanks @rhettinger for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 11, 2019
@rhettinger @miss-islington
bpo-37819: Add Fraction.as_integer_ratio() (pythonGH-15212) …
8a97a86 
(cherry picked from commit f03b4c8)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
@bedevere-bot

bedevere-bot commented Aug 11, 2019

Copy link
Copy Markdown

GH-15215 is a backport of this pull request to the 3.8 branch.

rhettinger added a commit that referenced this pull request Aug 11, 2019
@miss-islington @rhettinger
bpo-37819: Add Fraction.as_integer_ratio() (GH-15212) (GH-15215) …
5ba1cb0 
(cherry picked from commit f03b4c8)

Co-authored-by: Raymond Hettinger <rhettinger@users.noreply.github.com>
@jdemeyer

jdemeyer commented Aug 13, 2019

Copy link
Copy Markdown
Contributor

Should this as_integer_ratio instead be moved to the numbers.Rational base class? The implementation already makes sense on that level, so it seems more natural to do it that way.

@aeros aeros mentioned this pull request Aug 19, 2019
Closed
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@rhettinger @lisroach
bpo-37819: Add Fraction.as_integer_ratio() (pythonGH-15212)
d738b5c
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@rhettinger @DinoV
bpo-37819: Add Fraction.as_integer_ratio() (pythonGH-15212)
476a8a4
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
@rhettinger @websurfer5
bpo-37819: Add Fraction.as_integer_ratio() (pythonGH-15212)
91441a2
@skirpichev skirpichev mentioned this pull request Feb 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@methane methane methane left review comments

@gvanrossum gvanrossum gvanrossum left review comments

@mdickinson mdickinson Awaiting requested review from mdickinson

+1 more reviewer

@aeros aeros aeros left review comments

Assignees

No one assigned

Labels

type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@rhettinger @aeros @miss-islington @bedevere-bot @jdemeyer @methane @gvanrossum @the-knights-who-say-ni