Skip to content

[3.14] gh-122450: Indicate that Fraction denominators are always positive (GH-136789) #136792

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.

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

Closed

Conversation

miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jul 19, 2025

@python-cla-bot
Copy link

The following commit authors need to sign the Contributor License Agreement:

CLA signed

@AA-Turner
Copy link
Member

@nacind please could you sign the CLA here?

A

@AA-Turner
Copy link
Member

@serhiy-storchaka was "do-not-merge" intentional?

@skirpichev
Copy link
Contributor

was "do-not-merge" intentional?

See #136789 (review).

This change is incorrect and should be reverted.

@AA-Turner
Copy link
Member

I disagree that this is incorrect. At worst, I would say that it has an ambiguous interpretation. In general, I think statements like the above are generally less helpful, especially in cases like this where we are discussing linguistic merits of word choice and not a technical implementation. I'm happy to hold this PR in 'draft', though, whilst we continue to make improvements to the fractions documentation.

A

@AA-Turner AA-Turner marked this pull request as draft July 31, 2025 07:48
@skirpichev
Copy link
Contributor

I disagree that this is incorrect.

Two core devs disagree with you. I think it's a good argument against merging such changes and for a quick revert.

At worst, I would say that it has an ambiguous interpretation.

Well, it's math. You can't interpret same term differently in same sentence. That doesn't make any sense.

I accept, documentation can be later improved. But now it's just plain wrong: it asserts that denominator argument is positive.

@AA-Turner
Copy link
Member

it asserts that denominator argument is positive.

Our style guide notes that arguments are italicised. Indeed, that is the case here -- the denominator argument is stated as being a Rational and that if it is 0, an exception is raised. We then say that the Fraction object will have value equal to $numerator/denominator$. In that same part, we note that the denominator will be converted to a positive number. I contend that equal to is not identical to in this case, so noting that the resulting denominator will be positive is fine. I think the PR to further improve this is nearly ready, I've just made two suggestions within the last hour.

A

@AA-Turner
Copy link
Member

Closing this now in favour of #136800 (which will need manual backports).

A

@AA-Turner AA-Turner closed this Jul 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO-NOT-MERGE docs Documentation in the Doc dir skip news
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

5 participants