GeezerVison

bedevere-app Bot added the awaiting review label

bedevere-app Bot mentioned this pull request

format(Fraction(1, 3), '.016f') raises ValueError #130662

Closed

skirpichev commented Feb 28, 2025

Copy link

Copy Markdown

Member Author

CC @ericvsmith per experts index

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka approved these changes

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

I have some minor suggestions for tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_fractions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot added awaiting merge and removed awaiting review labels

skirpichev commented

View reviewed changes

Comment thread

Lib/test/test_fractions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum reviewed

View reviewed changes

Comment thread

Lib/fractions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum reviewed

gh-130662: refine width/precision specification in format-spec #130717

View reviewed changes

gvanrossum left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that there's another inconsistency between float and Fraction.

x = 1/3
f"{x:_>010f}"  # '__3.140000', ignores the zero-pad
x = Fraction(1, 3)
Traceback (most recent call last):
  File "<python-input-4>", line 1, in <module>
    f"{x:_>010f}"
      ^^^^^^^^^^
  File "/Users/guido/cpython/Lib/fractions.py", line 600, in __format__
    raise ValueError(
    ...<2 lines>...
    )
ValueError: Invalid format specifier '_>010f' for object of type 'Fraction'

So we still fail the goal of Fraction supporting everything that float does. Since Fraction actually is more strict (which is the way of the future) aren't we fixing this the wrong way?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Update Lib/test/test_fractions.py

9425ff2

skirpichev commented Mar 1, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

Here is implementation pr, for context: #100161

The ValueError raised in case both alignment (and the fill character) and zero padding are specified on the ground "refuse the temptation to guess". In principle, it's not hard to reproduce float's behavior (zero padding is ignored if the fill character is specified). Though, maybe it's better to align this with the Fraction instead. Probably, it's a separate issue. Edit: #130716

Few another incompatibilities: #130664

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum commented Mar 1, 2025

Copy link

Copy Markdown

Member

As happens occasionally, the docs are out of sync with the implementation, and we should respond by updating the docs, not the code. IMO.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev mentioned this pull request

Mar 1, 2025

Closed

skirpichev commented Mar 1, 2025

Copy link

Copy Markdown

Member Author

The problem is that the stdlib has several beasts (Decimal and Fraction), that have slightly different implementations of new-style formatting. IMO, documenting all these differences will make documentation much less readable.

Also, it's less obvious to which convention should follow external modules, like the mpmath or the gmpy2.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum commented Mar 1, 2025

Copy link

Copy Markdown

Member

Okay, I will leave it to more active core devs to sort out.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev commented Apr 7, 2025

Copy link

Copy Markdown

Member Author

Alternative pr: #130717

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner approved these changes

Apr 15, 2025

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I agree with @serhiy-storchaka: #130662 (comment)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

skirpichev changed the title ~~gh-130662: make Fraction's formatting more compatible wrt float's~~ gh-130662: accept leading zeros in precision/width for Fraction's formatting

Apr 15, 2025

skirpichev added the 3.14 bugs and security fixes label

Apr 28, 2025

skirpichev added needs backport to 3.14 bugs and security fixes and removed 3.14 bugs and security fixes labels

May 8, 2025

skirpichev requested a review from picnixz

June 2, 2025 06:29

picnixz reviewed

View reviewed changes

Comment thread

Lib/test/test_fractions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/fractions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

skirpichev added 2 commits

June 2, 2025 14:25


          Merge branch 'master' into fix-fmt-fractions/130662

d68c869


          address review: add news

e0d2526

skirpichev requested a review from picnixz

June 2, 2025 11:39

picnixz approved these changes

View reviewed changes

Comment thread

Misc/NEWS.d/next/Library/2025-06-02-14-28-30.gh-issue-130662.EIgIR8.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Apply suggestions from code review

a54c7e5

Co-authored-by: Bénédikt Tran <[email protected]>

vstinner merged commit 5bc2d99 into python:main

bedevere-app Bot removed the awaiting merge label

miss-islington-app Bot commented Jun 2, 2025

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request


          pythongh-130662: Accept leading zeros in precision/width for Fraction…

2df70e3

…'s formatting (pythonGH-130663)

(cherry picked from commit 5bc2d99)

Co-authored-by: Sergey B Kirpichev <[email protected]>
Co-authored-by: Bénédikt Tran <[email protected]>

bedevere-app Bot commented Jun 2, 2025

Copy link

Copy Markdown

GH-135026 is a backport of this pull request to the 3.14 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label

vstinner commented Jun 2, 2025

Copy link

Copy Markdown

Member

Merged, thank you.

GH-135026 is a backport of this pull request to the 3.14 branch.

I disagree: we should not backport this change to stable branches.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

picnixz commented Jun 2, 2025

Copy link

Copy Markdown

Member

Wasn't it a fix for something we introduced in 3.14? @skirpichev

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Jun 2, 2025

Copy link

Copy Markdown

Member

Wasn't it a fix for something we introduced in 3.14? @skirpichev

No, Python 3.13 has the same behavior for example. It was "always" like that.

@skirpichev proposed to have the behavior for all types: float, Fraction, Decimal. Before, only float accepted leading zeros.

picnixz reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev commented Jun 2, 2025

Copy link

Copy Markdown

Member Author

I disagree: we should not backport this change to stable branches.

Why not? It's a bug, isn't?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev deleted the fix-fmt-fractions/130662 branch

June 2, 2025 13:56

vstinner commented Jun 2, 2025

Copy link

Copy Markdown

Member

Why not? It's a bug, isn't?

From my point of view, it's a new feature and existing projects may rely on the current (Python 3.14) behavior.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev commented Jun 2, 2025

Copy link

Copy Markdown

Member Author

From my point of view, it's a new feature and existing projects may rely on the current (Python 3.14) behavior.

How?!

The issue is about formatting stdlib types (Fractions and Decimals, in a separate pr). Should external projects use current formatting specification mini-language or use more strict rules? This was coming from mpmath/mpmath#915.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka commented Jun 2, 2025

Copy link

Copy Markdown

Member

From my point of view, this is a borderline between a minor feature and an unsignificant bug fix which we may not want to backport. So maybe to 3.14 only? The solved problem is rather scholastic.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

skirpichev mentioned this pull request

Add underscore as a decimal separator for string formatting #87790

Closed

serhiy-storchaka added the needs backport to 3.14 bugs and security fixes label

miss-islington-app Bot commented Jul 7, 2025

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request


          pythongh-130662: Accept leading zeros in precision/width for Fraction…

c53c357

…'s formatting (pythonGH-130663)

(cherry picked from commit 5bc2d99)

Co-authored-by: Sergey B Kirpichev <[email protected]>
Co-authored-by: Bénédikt Tran <[email protected]>

bedevere-app Bot commented Jul 7, 2025

Copy link

Copy Markdown

GH-136361 is a backport of this pull request to the 3.14 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot removed the needs backport to 3.14 bugs and security fixes label

hugovk pushed a commit that referenced this pull request