bpo-23975: Correct conversion of Rational’s to float

[skirpichev]

Also document that numerator/denominator properties are instances of the Integral.

This solution was suggested by Paul Moore.

https://bugs.python.org/issue23975

[github-actions]

This PR is stale because it has been open for 30 days with no activity.

[skirpichev]

@mdickinson, you are listed as an expert for the fractions module, could you review this little PR? (This, probably, lacks a test.)

[mdickinson]

@skirpichev Seems like a reasonable approach to me. But as you say, it needs a test before it can go in. (There should be at least something in the test suite that fails if those int conversions are removed.)

[skirpichev]

But as you say, it needs a test before it can go in.

Ok, lets see how it looks.

[skirpichev]

Hmm, unfortunately, there is a silent type-cast. I can adjust the test, but may be it does make sense to add

diff --git a/Lib/fractions.py b/Lib/fractions.py
index f9ac882ec0..4a153ee8e3 100644
--- a/Lib/fractions.py
+++ b/Lib/fractions.py
@@ -143,6 +143,10 @@ def __new__(cls, numerator=0, denominator=None, *, _normalize=True):
         elif type(numerator) is int is type(denominator):
             pass # *very* normal case
 
+        elif (isinstance(numerator, numbers.Integral) and
+            isinstance(denominator, numbers.Integral)):
+            pass
+
         elif (isinstance(numerator, numbers.Rational) and
             isinstance(denominator, numbers.Rational)):
             numerator, denominator = (

... or I can just "fix" type of private _numerator/denominator properties in the test by monkey patching.

[skirpichev]

@mdickinson, let me know if you have some objections for added tests (maybe Lib/test/test_abstract_numbers.py is the right place to this?). I've realized that there almost no tests for default methods of the numbers.py (#32022 address this issue, partially).

[MaxwellDupre]

The fix looks simple but where does the current implementation fail? Can you provide a test case such that the current fails but the after the fix the test succeeds?

[skirpichev]

@MaxwellDupre, the added test should be such one. At least it was a failing test for the old main branch (~Apr 2021).

[MaxwellDupre]

LGTM

[mdickinson]

LGTM

[miss-islington]

Thanks @skirpichev for the PR, and @mdickinson for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10, 3.11.
🐍🍒⛏🤖

[bedevere-bot]

GH-96556 is a backport of this pull request to the 3.11 branch.

[bedevere-bot]

GH-96557 is a backport of this pull request to the 3.10 branch.