FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-95504: Fix sign placement in PyUnicode_FromFormat by philg314 · Pull Request #95505 · python/cpython · GitHub

/ cpython Public

gh-95504: Fix sign placement in PyUnicode_FromFormat - #95505

Closed
philg314 wants to merge 8 commits into
python:mainfrom
philg314:unicode-from-format-negative
ConversationCommits (8)ChecksFiles changed
Closed

gh-95504: Fix sign placement in PyUnicode_FromFormat#95505
philg314 wants to merge 8 commits into
python:mainfrom
philg314:unicode-from-format-negative

Conversation

philg314 commented Jul 31, 2022 •
edited
Loading

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

ghost commented Jul 31, 2022 •
edited by ghost
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

corona10 requested a review from methane August 1, 2022 03:03
methane approved these changes Aug 1, 2022
View reviewed changes

methane left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM.
But this is bit sensitive to backport because Python 3.11 is right before RC.

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.

philg314 marked this pull request as draft August 1, 2022 15:33
philg314 marked this pull request as ready for review August 2, 2022 03:28
philg314 requested a review from a team as a code owner August 2, 2022 03:28

philg314 commented Aug 6, 2022

Copy link
Copy Markdown
Contributor Author

@encukou this is ready for review.

serhiy-storchaka requested review from serhiy-storchaka and removed request for a team August 7, 2022 08:50
serhiy-storchaka reviewed Aug 7, 2022
View reviewed changes

serhiy-storchaka left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM in general, but I would wait for testcapi refactoring.

Comment thread Misc/ACKS
Comment thread Objects/unicodeobject.c Outdated
Comment thread Objects/unicodeobject.c
Comment on lines +2507 to +2510
if (negative && !zeropad) {
if (_PyUnicodeWriter_WriteChar(writer, '-') == -1)
return NULL;
}

serhiy-storchaka Aug 7, 2022

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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().

philg314 Aug 7, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

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.

This was referenced Aug 9, 2022
Merged
Merged

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?

encukou commented Aug 10, 2022

Copy link
Copy Markdown
Member

The changes were merged in #95819 and #95848.

Thanks for the contribution, @philg314!

encukou closed this Aug 10, 2022

philg314 commented Aug 10, 2022

Copy link
Copy Markdown
Contributor Author

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

philg314 deleted the unicode-from-format-negative branch August 10, 2022 14:28
encukou mentioned this pull request Aug 15, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Assignees

No one assigned

Labels

awaiting merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL