GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-95504: Fix sign placement in PyUnicode_FromFormat by philg314 · Pull Request #95505 · python/cpython

Conversation

@philg314

@philg314 philg314 commented Jul 31, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

gh-95504: Fix sign placement in PyUnicode_FromFormat

The sign doesn't stick to the number anymore:
PyUnicode_FromFormat("%05d", -123); results in "-0123" instead of "0-123",
PyUnicode_FromFormat("%.5d", -123); results in "-00123" instead of "0-123".

  • Issue: Wrong sign placement in PyUnicode_FromFormat #95504

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
philg314 added 2 commits July 31, 2022 21:21
@philg314
Add tests for PyUnicode_FromFormat
2d6f3e1
@philg314
Fix negative numbers in PyUnicode_FromFormat
bd72e2a
@ghost

ghost commented Jul 31, 2022 •
edited by ghost
Loading

Uh oh!

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

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.
CLA signed
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting review label Jul 31, 2022
@corona10
corona10 requested a review from methane August 1, 2022 03:03
methane
methane approved these changes Aug 1, 2022
View reviewed changes

@methane methane 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.
But this is bit sensitive to backport because Python 3.11 is right before RC.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Aug 1, 2022
@philg314

philg314 commented Aug 1, 2022

Copy link
Copy Markdown
Contributor Author

I emailed core-mentorship about this on Friday night but didn't wait to hear back before submitting this PR.

@encukou is in the process of splitting _testcapimodule.c up into more manageable pieces (#93649) and has asked me to move Unicode related tests into a separate file. I'll convert this PR to a draft while working on that and convert it back once that's done.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@philg314
philg314 marked this pull request as draft August 1, 2022 15:33
philg314 added 3 commits August 2, 2022 05:04
@philg314
Merge remote-tracking branch 'upstream/main' into unicode-from-format…
95b4f83 
…-negative
@philg314
Add whatsnew
99b0ebc
@philg314
pythongh-93649: Split unicode tests from _testcapimodule.c
63e9afd
@philg314
philg314 marked this pull request as ready for review August 2, 2022 03:28
@philg314
philg314 requested a review from a team as a code owner August 2, 2022 03:28
philg314 added 2 commits August 6, 2022 13:00
@philg314
Use built-in AssertionError
d70fa1f
@philg314
Merge remote-tracking branch 'upstream/main' into unicode-from-format…
79a8138 
…-negative
@philg314

philg314 commented Aug 6, 2022

Copy link
Copy Markdown
Contributor Author

@encukou this is ready for review.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka
serhiy-storchaka requested review from serhiy-storchaka and removed request for a team August 7, 2022 08:50
serhiy-storchaka
serhiy-storchaka reviewed Aug 7, 2022
View reviewed changes

@serhiy-storchaka 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 in general, but I would wait for testcapi refactoring.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Misc/ACKS Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/unicodeobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/unicodeobject.c
Comment on lines +2507 to +2510
if (negative && !zeropad) {
if (_PyUnicodeWriter_WriteChar(writer, '-') == -1)
return NULL;
}

@serhiy-storchaka serhiy-storchaka Aug 7, 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.

It can be simpler. Instead of writing minus explicitly, you can write it as a part of the buffer.

You will get the same result if remove this if and pass &buffer[negative && zeropad] instead of &buffer[negative] to _PyUnicodeWriter_WriteASCIIString().

Sorry, something went wrong.

Uh oh!

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

@philg314 philg314 Aug 7, 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.

That wouldn't work with precision: %.5d with -123 would be 00-123.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@philg314
Implement requested changes
e8d0edf
@encukou

encukou commented Aug 8, 2022

Copy link
Copy Markdown
Member

Sorry for making you do more work, but it works best if the test refactoring is done first, in a separate pull request, so that:

  • the change PR shows what changed in the tests
  • if something goes wrong, the change can be reverted without the refactoring
  • the refactoring can be applied to older unfixed versions, if necessary (e.g. wep only need to backport a subset of changes)

Do you want to split this up? I can also do it, and it wouldn't be too much extra work in addition to a review, but I don't want to “steal your contribution”.
All reactions

Sorry, something went wrong.

Uh oh!

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

philg314 commented Aug 8, 2022

Copy link
Copy Markdown
Contributor Author

Sorry for making you do more work, but it works best if the test refactoring is done first, in a separate pull request, so that:

  • the change PR shows what changed in the tests
  • something goes wrong, the change can be reverted without the refactoring
  • refactoring can be applied to older unfixed versions, if necessary (e.g. wep only need to backport a subset of changes)

No problem, makes sense.

Do you want to split this up? I can also do it, and it wouldn't be too much extra work in addition to a review, but I don't want to “steal your contribution”.

Sure, and for future reference I'll never have a problem with anything like that. Please ping me when the refactor is merged.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
This was referenced Aug 9, 2022
gh-93649: Split unicode tests from _testcapimodule.c & add some more #95819
Merged
gh-95504: Fix negative numbers in PyUnicode_FromFormat #95848
Merged
@encukou

encukou commented Aug 10, 2022

Copy link
Copy Markdown
Member

@philg314,
Tests were merged in #95819.
The behavior change is in #95848, could you check everything is in order?
All reactions

Sorry, something went wrong.

Uh oh!

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

encukou commented Aug 10, 2022

Copy link
Copy Markdown
Member

The changes were merged in #95819 and #95848.

Thanks for the contribution, @philg314!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou encukou closed this Aug 10, 2022
@philg314

philg314 commented Aug 10, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for the reviews @methane and @serhiy-storchaka and thanks for merging @encukou!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@philg314
philg314 deleted the unicode-from-format-negative branch August 10, 2022 14:28
@encukou encukou mentioned this pull request Aug 15, 2022
Test Limited API in _testcapi module #95991
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments
@methane methane methane approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

awaiting merge

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

@philg314 @encukou @methane @serhiy-storchaka @bedevere-bot
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 01402503 — 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.