Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-39350: Fix regression introduced when fractions.gcd was removed #18309
Conversation
This comment has been minimized.
This comment has been minimized.
The broken code made it to 3.9.0a3. That's how I have discovered the regression. |
This comment has been minimized.
This comment has been minimized.
Whoops; you're right. News entry needed, then! |
This comment has been minimized.
This comment has been minimized.
codecov
bot
commented
Feb 2, 2020
Codecov Report
@@ Coverage Diff @@
## master #18309 +/- ##
===========================================
+ Coverage 82.11% 83.19% +1.07%
===========================================
Files 1955 1570 -385
Lines 588596 414126 -174470
Branches 44401 44402 +1
===========================================
- Hits 483346 344525 -138821
+ Misses 95605 59963 -35642
+ Partials 9645 9638 -7
Continue to review full report at Codecov.
|
| @@ -13,6 +13,12 @@ | |||
| __all__ = ['Fraction'] | |||
|
|
|||
|
|
|||
| def _gcd(a, b): | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
corona10
Feb 2, 2020
•
Member
-def _gcd(a, b):
- # Supports non-integers for backward compatibility.
- while b:
- a, b = b, a%b
- return a
-
# Constants related to the hash implementation; hash(x) is based
# on the reduction of x modulo the prime _PyHASH_MODULUS.
_PyHASH_MODULUS = sys.hash_info.modulus
@@ -98,6 +92,11 @@ class Fraction(numbers.Rational):
"""
self = super(Fraction, cls).__new__(cls)
+ def _gcd(a, b):
+ while b:
+ a, b = b, a%b
+ return a
+
if denominator is None:
if type(numerator) is int:
self._numerator = numerator
This comment has been minimized.
This comment has been minimized.
mdickinson
Feb 2, 2020
Author
Member
Looks good, what about declared this function as the nested function?
I'd prefer to revert to as close as possible to the previous code, which had _gcd as a module-level function.
I think we do need a news entry, because this is fixing a bug that was already made available in a release. If the bug had been introduced and then fixed without any intermediate release, then we could have skipped the news entry.
This comment has been minimized.
This comment has been minimized.
mdickinson commentedFeb 2, 2020
•
edited by bedevere-bot
This PR is a quick fix for the breakage introduced in #18021, which removed the
_gcdfunction while the core code was still using it.Possibly the behaviour of
Fractionfor non-integers should be different, but I think it makes sense to fix the breakage immediately and then discuss whether we want to make behavioural changes.No news entry needed, because the broken code hasn't make it into any release or prerelease.
https://bugs.python.org/issue39350