Skip to content

gh-122450: Indicate that Fraction denominators are always positive #136789

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

Merged
AA-Turner merged 1 commit into from
Jul 19, 2025
Merged

gh-122450: Indicate that Fraction denominators are always positive #136789

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Doc/library/fractions.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ another rational number, or from a string.

The first version requires that *numerator* and *denominator* are instances
of :class:`numbers.Rational` and returns a new :class:`Fraction` instance
with value ``numerator/denominator``. If *denominator* is ``0``, it
raises a :exc:`ZeroDivisionError`.
with value equal to ``numerator/denominator`` where the denominator is positive.

@skirpichev skirpichev Jul 19, 2025 •
edited
Loading

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.

Sorry, I don't think it's an improvement.

Old sentence is clear: we return a rational number with value numerator/denominator.
But original numerator and denominator may have any sign, may be not integers, etc.

@serhiy-storchaka serhiy-storchaka Jul 19, 2025

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.

I agree with @skirpichev. We can leave "equals", but "where the denominator is positive" is incorrect or at least ambiguous. "Denominator" refers to the argument of the Fraction constructor, not the denominator property.

@picnixz picnixz Jul 19, 2025 •
edited
Loading

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.

I also agree. It's as if we require denomintor to be positive (as an argument) which is not the case. So I suggest to revert this PR.

@skirpichev skirpichev Jul 19, 2025

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.

I opened pr to fix docstrings in Rational class: #136800 It's a accompanying pr to #122464, which fix sphinx docs (and stuck in review).

But maybe it's ok to combine both and include reversion of this pr? All that only affects docs and IMO - fine for backporting up to 3.13.

What do you think?

@AA-Turner AA-Turner Jul 19, 2025

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.

@serhiy-storchaka @skirpichev we discussed this at the sprint and decided that *denominator* (italics) referred to the argument, but denominator (normal/roman) in the context of the sentence referred to the new argument. Regardless, it simplifies the arguments, so it is not always the denominator argument that is set as the class attribute.

A

@serhiy-storchaka serhiy-storchaka Jul 19, 2025

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.

It says: "a new :class:Fraction instance with value equal to numerator/denominator". "denominator" refers to argument, not to the property. Otherwise this sentence does not make sense. Then you say that in "where the denominator is positive" "denominator" refers to something other. But it clearly refers to the same thing as in previous expression. Your interpretation requires interpreting the same word in two different ways simultaneously. Even if it makes sense, it is very unclear and confusing. And to me, it just looks wrong.

If *denominator* is ``0``, it raises a :exc:`ZeroDivisionError`.

The second version requires that *number* is an instance of
:class:`numbers.Rational` or has the :meth:`!as_integer_ratio` method
Expand Down
Loading