I noticed that there was a single line of Lib/fractions.py that did not have test coverage: the normalize step for fractions with non-integer numerators and/or denominators.

I initially was going to implement a test for that line, but upon further reflection, I cannot envision a scenario where that would be possible. The code already requires its initial parameters to be numbers.Rational and then normalizes their numerators and denominators to be integers.

So, instead, I propose to remove this check, first introduced in issue22486, and to roll the fractions._gcd() function up into the deprecated fractions.gcd().

> [...] I cannot envision a scenario where that would be possible [...]

I don't think it can be ruled out. If I'm reading the code right, it's preceded by this branch of the initial if/elif chain:

        elif (isinstance(numerator, numbers.Rational) and
            isinstance(denominator, numbers.Rational)):
            numerator, denominator = (
                numerator.numerator * denominator.denominator,
                denominator.numerator * numerator.denominator
                )

I don't think there's any guarantee that if `numerator` is an instance of `numbers.Rational`, then `numerator.numerator` and `numerator.denominator` have exact type `int`.  (Note that an instance of `numbers.Rational` is not necessarily an instance of `fractions.Fraction`.)

Indeed, that is the code fragment I was referring to.

Mathematically speaking, a rational number is one that can be expressed as a fraction of two integers, so in that regard the numerator and the denominator should both be integers.

But let's assume for argument's sake that a type comes through where the numerator and the denominator are fractions: 1/2 and 2/3, respectively. This code would normalize them by cross-multiplying:

numerator = 1 * 3 = 3
denominator = 2 * 3 = 6

Now they are both integers.

In what scenario would the numerator and denominator be numbers.Rational but not an integer or a fraction? If someone could even come up with one, would it be worthwhile to allow as a fraction?

And on the flip side, if math.gcd() only accepts integers and not, at least, numbers.Integral, wouldn't that be a bug?

> In what scenario would the numerator and denominator be numbers.Rational but not an integer or a fraction

But that's not the issue here. The issue here is having an instance of `numbers.Rational` whose numerator and denominator are not specifically of concrete type `int`: the test that (IIUC) you're proposing to remove is:

     if type(numerator) is int is type(denominator):
         [...]

Certainly I'd expect the numerator and denominator to be instances of `numbers.Integral`, but that's more general than being of exact type `int`.

The `numbers.Integral` and `numbers.Rational` ABCs are deliberately not tied to particular concrete classes. It would be perfectly possible (and reasonable) for someone to have a `MyFraction` class that behaves like a `Fraction`, and whose numerator and denominator are instances of some other concrete class `MyInteger` that behaves like an integer.

The branch in the code is necessary to support that situation, so we can't simply remove it.

Here's a concrete example, using gmpy2 [1], that exercises the code path you're proposing to remove. I had to cheat a bit, since the gmpy2 types *don't* (currently) buy in to the numbers ABC tower (though there's no good reason that they couldn't do so), so I registered them manually.

>>> import fractions, numbers
>>> import gmpy2
>>> x = gmpy2.mpq(2, 3)
>>> numbers.Rational.register(type(x))
<class 'mpq'>
>>> numbers.Integral.register(type(x.denominator))
<class 'mpz'>
>>> fractions.Fraction(x, x)  # code path exercised here ...
Fraction(1, 1)

Note that the numerator and the denominator of the resulting `Fraction` object are still of type `mpz`.

It _would_ be possible to remove the check and always use `math.gcd`, but it would result in a behaviour change: for the above code, we'd get a `Fraction` whose numerator and denominator were both of actual type `int` instead of `mpz`. Whether that's a desirable behaviour change or not is another question ...

[1] https://gmpy2.readthedocs.io/en/latest/

> if math.gcd() only accepts integers and not, at least, numbers.Integral, wouldn't that be a bug?

I'd call it an enhancement opportunity rather than a bug. :-) There's no general Python-wide requirement that an instance of numbers.Integral should be acceptable anywhere an int is (though I agree that it's a nice property to have in general).

But as it happens, math.gcd _does_ accept instances of numbers.Integral, since any such instance should implement the __index__ method to convert itself to a plain old int, and math.gcd passes its arguments through PyNumber_Index.

> but it would result in a behaviour change: for the above code, we'd get a `Fraction` whose numerator and denominator were both of actual type `int` instead of `mpz`

Ah, sorry. That's not true in this particular case. The returned gcd would be of type `int`, but then we end up dividing an `mpz` by an `int` to get an `mpz`. So I guess the problem case is where we have something that's like `mpz` but that doesn't support division by a plain `int`: I don't think there's any requirement that instances of numbers.Integral support mixed-type arithmetic with plain ints.

I'd actually be happier if the `fractions.Fraction` type *did* normalise so that its numerator and denominator always had exact type `int`, but that's not the way that it was designed: it was designed to support arbitrary numbers.Integral instances.

Side note: https://github.com/aleaxit/gmpy/issues/127 suggests that the types in question were added to the numeric tower for gmpy 2.0.9 and 2.1.0.

So, if I'm understanding your position correctly:

* We're back to needing a test for the line in question.
* We're eschewing the possibility of changing the behavior of `fractions.Fraction` to force int numerator and denominator.

Is that correct?

> the types in question were added to the numeric tower for gmpy 2.0.9 and 2.1.0.

Ah, good to know. Thanks. I was using 2.0.8.

> * We're back to needing a test for the line in question.

Agreed. It's not currently covered, and with the current behaviour the line _is_ necessary.

> * We're eschewing the possibility of changing the behavior of `fractions.Fraction` to force int numerator and denominator.

I think that's a separate issue from this one, and if you want to pursue it it's worth its own issue on the tracker. But there would need to be a good reason for deliberately changing the existing by-design behaviour.

I've retitled the issue to avoid confusion, since we're no longer proposing removing the _gcd function.

Mark, I think the Lib/fractions.py now is covered by tests in the test_fractions.py.

$ ./python -m venv ~/venv/cpython
$ . ~/venv/cpython/bin/activate
$ pip install coverage
Collecting coverage
  Using cached coverage-5.5-cp310-cp310-manylinux1_x86_64.whl (238 kB)
Installing collected packages: coverage
Successfully installed coverage-5.5
$ python -m coverage run --source=fractions --branch Lib/test/test_fractions.py
...............................
----------------------------------------------------------------------
Ran 31 tests in 0.040s

OK
$ ~/venv/cpython/bin/python -m coverage report
Name               Stmts   Miss Branch BrPart  Cover
----------------------------------------------------
Lib/fractions.py     299      0    126      0   100%
----------------------------------------------------
TOTAL                299      0    126      0   100%

My guess: this issue can be closed.

Thanks, Sergey! Agreed that this can be closed.