GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-45995: add "z" format specifer to coerce negative 0 to zero by belm0 · Pull Request #30049 · python/cpython

Conversation

belm0 commented Dec 11, 2021 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

This option coerces negative zero into zero after rounding to the format precision.
Covers formatting of standard fraction types (float, Decimal, complex) via str.format(), built-in format(), and f-strings. Old-style string interpolation
is not supported.

https://www.python.org/dev/peps/pep-0682/

https://bugs.python.org/issue45995

Sorry, something went wrong.

Uh oh!

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

All reactions

belm0 requested a review from isidentical as a code owner

December 11, 2021 13:24

the-knights-who-say-ni added the CLA signed label

Dec 11, 2021

bedevere-bot added the awaiting review label

Dec 11, 2021

belm0 marked this pull request as draft

December 11, 2021 13:24

belm0 force-pushed the format_negative_zero branch from 97a033b to 8ff811f Compare

December 11, 2021 13:44

belm0 marked this pull request as ready for review

December 14, 2021 13:23

belm0 requested a review from rhettinger as a code owner

December 14, 2021 13:23

belm0 commented

Dec 18, 2021

View reviewed changes

Comment thread

Modules/_decimal/_decimal.c Outdated

Comment on lines 3252 to 3263

belm0 Dec 18, 2021

Copy link

Copy Markdown

Contributor Author

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.

if PR #29438 lands, then detection of the "z" option can be incorporated into our forked mpd_parse_fmt_str_ex(), rather than this preprocessing of the format string

Sorry, something went wrong.

Uh oh!

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

mdickinson reacted with thumbs up emoji

All reactions

👍 1 reaction

ericvsmith Mar 15, 2022

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.

I think it's the case that one way or the other, the format spec is going to need to be completely parsed to figure out if a given 'z' is used for no_neg_0.

Sorry, something went wrong.

Uh oh!

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

All reactions

belm0 commented

Dec 18, 2021

View reviewed changes

Comment thread

Lib/_pydecimal.py Resolved

Uh oh!

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

github-actions Bot commented Jan 18, 2022

Copy link

Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

All reactions

Sorry, something went wrong.

Uh oh!

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

github-actions Bot added the stale Stale PR or inactive for long period of time. label

Jan 18, 2022

taleinat requested a review from mdickinson

January 18, 2022 07:38

mdickinson removed the stale Stale PR or inactive for long period of time. label

Jan 22, 2022

mdickinson commented Jan 22, 2022

Copy link

Copy Markdown

Member

Removing the stale label; discussion is ongoing on the bug tracker.

All reactions

Sorry, something went wrong.

Uh oh!

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

belm0 force-pushed the format_negative_zero branch from 1bfb289 to bb4d5f1 Compare

February 4, 2022 02:46

mdickinson commented Mar 6, 2022

Copy link

Copy Markdown

Member

Update: the PEP has been accepted - https://discuss.python.org/t/accepting-pep-682-format-specifier-for-signed-zero/14088

We should aim to get this reviewed and in by the next (and final) alpha release of 3.11, due April 5th.

All reactions

Sorry, something went wrong.

Uh oh!

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

mdickinson suggested changes

Mar 13, 2022

View reviewed changes

mdickinson 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.

Hi @belm0. Here's a first-pass review; apologies for taking so long to get to this.

Most of the comments are nitpick-level. The big exception is the Decimal code, where I think there's still significant work to do.

Sorry, something went wrong.

Uh oh!

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

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

bedevere-bot removed the awaiting review label

Mar 13, 2022

bedevere-bot commented Mar 13, 2022

Copy link

Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot added the awaiting changes label

Mar 13, 2022

mdickinson commented Mar 13, 2022

Copy link

Copy Markdown

Member

@ericvsmith: if you have any bandwidth to look over the code at some point before the first 3.11 beta, I'd appreciate your input.

All reactions

Sorry, something went wrong.

Uh oh!

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

ericvsmith reviewed

Mar 13, 2022

View reviewed changes

Comment thread

Misc/NEWS.d/next/Library/2021-12-14-13-15-41.bpo-45995.Am9pNL.rst Resolved

Uh oh!

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

belm0 commented

Mar 15, 2022

View reviewed changes

belm0 left a comment

Copy link

Copy Markdown

Contributor Author

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.

Thank you for the thorough review!

I'm bogged down a bit and it may take a week or so to reconcile the comments.

Hoping to get this merged for the next (final) alpha.

3.11.0 alpha 7: Tuesday, 2022-04-05
3.11.0 beta 1: Friday, 2022-05-06

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_float.py Resolved

Uh oh!

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

belm0 and others added 7 commits

March 19, 2022 14:57


          bpo-45995: add "z" format specifer to coerce negative 0 to zero

a633e88

This covers str.format() and f-strings.  Old-style string
interpolation is not supported.

TODO: Decimal support


          formatting

636da05


          implementation for Decimal

3f4085d


          📜🤖 Added by blurb_it.

68a049e


          consistent flag names

ad32be5


          add test case for integer value with z option

3dcaf5f


          reference pending PEP

6b9ab3b


          clarify Decimal strategy

2a24e61

belm0 commented

Mar 23, 2022

View reviewed changes

Comment thread

Modules/_decimal/_decimal.c

Comment on lines +3360 to +3363

		if (no_neg_0 && mpd_isnegative(mpd) && !mpd_isspecial(mpd)) {
		/* Round into a temporary (carefully mirroring the rounding
		of mpd_qformat_spec()), and check if the result is negative zero.
		If so, clear the sign and format the resulting positive zero. */

belm0 Mar 23, 2022

Copy link

Copy Markdown

Contributor Author

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.

An insight I had recently was that, once negative zero is confirmed somehow, producing the correct output is only a matter of formatting positive zero (which cannot be affected by directed rounding, etc.).

As for how to detect negative zero, the options are: 1) round independently of the format function, or 2) analyze the string result of the format function (as Mark suggested). I found the former easier to manage, since it's a matter of duplicating a small amount of proven mpd library code, and is relatively straightforward to test. In contrast, analyzing the format output involves writing a new and perfectly-correct regex or parser, and imagining all the format spec edge cases to test it with.

Sorry, something went wrong.

Uh oh!

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

All reactions

mdickinson reviewed

Apr 5, 2022

View reviewed changes

mdickinson 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.

Here's a second-pass review. We're getting there, but there are still a couple of bugs to resolve.

Sorry, something went wrong.

Uh oh!

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

belm0 reacted with heart emoji

All reactions

❤️ 1 reaction

Comment thread

Lib/test/test_float.py Resolved

Uh oh!

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

Comment thread

Python/formatter_unicode.c Resolved

Uh oh!

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

Comment thread

Modules/_decimal/_decimal.c Resolved

Uh oh!

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

Comment thread

Modules/_decimal/_decimal.c Resolved

Uh oh!

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

Comment thread

Modules/_decimal/_decimal.c Resolved

Uh oh!

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

belm0 and others added 3 commits

April 7, 2022 09:14


          fix Decimal format parsing

0cbff6a


          fix Decimal when no precision is specified

8e7b51c


          fix comment typo

418ab76

Co-authored-by: Mark Dickinson <[email protected]>

belm0 requested a review from mdickinson

April 7, 2022 12:52

mdickinson approved these changes

Apr 11, 2022

View reviewed changes

mdickinson 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.

Thank you for the updates! This looks ready to merge to me. I've been running some automated tests that cover a wide variety of combinations, and haven't yet managed to break this. :-)

A bit off-topic: I do think we're introducing some DRY-type technical debt into _decimal.c, though for this PR I don't see an easy alternative. For the future, there's scope for some cleanup after lifting the formatting machinery from libmpdec to _decimal.c. That's for another day, though.

Sorry, something went wrong.

Uh oh!

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

belm0 reacted with thumbs up emoji

All reactions

👍 1 reaction

bedevere-bot added awaiting merge and removed awaiting change review labels

Apr 11, 2022

ericvsmith reviewed

Apr 11, 2022

View reviewed changes

Comment thread

Misc/NEWS.d/next/Library/2021-12-14-13-15-41.bpo-45995.Am9pNL.rst Resolved

Uh oh!

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

ericvsmith approved these changes

Apr 11, 2022

View reviewed changes

ericvsmith 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.

Other than possibly adding a credit in the blurb, this looks good to me.

Sorry, something went wrong.

Uh oh!

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

All reactions


          add attribution to news blurb

3ee6f6b

ghost commented Apr 11, 2022

Copy link

Copy Markdown

Commit authors are required to sign the Contributor License Agreement.

All reactions

Sorry, something went wrong.

Uh oh!

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

belm0 commented Apr 11, 2022

Copy link

Copy Markdown

Contributor Author

using file edit from the github UI appears to have angered CLA bot and bedevere

All reactions

Sorry, something went wrong.

Uh oh!

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

ericvsmith commented Apr 11, 2022

Copy link

Copy Markdown

Member

using file edit from the github UI appears to have angered CLA bot and bedevere

I think it's related to the bpo to github issues migration. I'll see what I can figure out.

All reactions

Sorry, something went wrong.

Uh oh!

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

ericvsmith added skip issue CLA signed and removed CLA signed labels

Apr 11, 2022

mdickinson commented Apr 11, 2022

Copy link

Copy Markdown

Member

The "CLA Signing" check doesn't seem to be required for merging. Given that we know that CLA signing has occurred, I think we should be okay to merge. I've reported the failure to @ambv.

All reactions

Sorry, something went wrong.

Uh oh!

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

mdickinson merged commit b0b836b into python:main

Apr 11, 2022

bedevere-bot removed the awaiting merge label

Apr 11, 2022

mdickinson added a commit to mdickinson/peps that referenced this pull request

Apr 11, 2022


          Mark PEP 682 as final.

89fcfb2

The implementation of PEP 682 was completed in python/cpython#30049.

mdickinson mentioned this pull request

Apr 11, 2022

PEP 682: Change status to Final python/peps#2513

Merged

belm0 deleted the format_negative_zero branch

April 11, 2022 14:40

JelleZijlstra pushed a commit to python/peps that referenced this pull request

Apr 11, 2022


          PEP 682: Mark as final (#2513)

2c4d71f

The implementation of PEP 682 was completed in python/cpython#30049.

mdickinson mentioned this pull request

Apr 11, 2022

string formatting: normalize negative zero #90153

Closed

navytux mentioned this pull request

Apr 30, 2023

"z" format specifier is treated differently in unicode and bytes #104018

Closed

ericvsmith mentioned this pull request

Jan 25, 2024

Possible memory leak in _cdecimal.c with 'z' format #114563

Closed

Reviewers

ericvsmith

ericvsmith approved these changes

isidentical

Awaiting requested review from isidentical

rhettinger

Awaiting requested review from rhettinger

+1 more reviewer

mdickinson

mdickinson approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

skip issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

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

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

belm0 commented Dec 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

belm0 Dec 18, 2021

Choose a reason for hiding this comment

Uh oh!

ericvsmith Mar 15, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jan 18, 2022

Uh oh!

mdickinson commented Jan 22, 2022

Uh oh!

mdickinson commented Mar 6, 2022

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Mar 13, 2022

Uh oh!

mdickinson commented Mar 13, 2022

Uh oh!

Uh oh!

belm0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

belm0 Mar 23, 2022

Choose a reason for hiding this comment

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdickinson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ericvsmith left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Apr 11, 2022

Uh oh!

belm0 commented Apr 11, 2022

Uh oh!

ericvsmith commented Apr 11, 2022

Uh oh!

mdickinson commented Apr 11, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

belm0 commented Dec 11, 2021 •

edited

Loading

Uh oh!

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