GeezerVison

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

GeezerVison is showing github.com — gh-98836: Extend PyUnicode_FromFormat() by serhiy-storchaka · Pull Request #98838 · python/cpython

Conversation

serhiy-storchaka commented Oct 29, 2022 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

Support for conversion specifiers o (octal) and X (uppercase hexadecimal).
Support for length modifiers j (intmax_t) and t (ptrdiff_t).
Length modifiers are now applied to all integer conversions.
Support for wchar_t C strings (%ls and %lV).
Support for variable width and precision (*).
Support for flag - (left alignment).

Issue: Extend PyUnicode_FromFormat() #98836

Sorry, something went wrong.

Uh oh!

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

All reactions


          pythongh-98836: Extend PyUnicode_FromFormat()

aca2393

* Support for conversion specifiers o (octal) and X (uppercase hexadecimal).
* Support for length modifiers j (intmax_t) and t (ptrdiff_t).
* Length modifiers are now applied to all integer conversions.
* Support for wchar_t C strings (%ls and %lV).
* Support for variable width and precision (*).
* Support for flag - (left alignment).

bedevere-bot added the awaiting core review label

Oct 29, 2022

serhiy-storchaka added 5 commits

October 29, 2022 17:48


          Fix docs formatting.

4fdf596


          Add tests for integer conversions.

13d4c97


          Merge branch 'main' into capi-unicode-fromformat

e69d8b0


          Add more tests.

3bd9d3e


          Finish documentation.

1e6094b

bedevere-bot mentioned this pull request

Nov 3, 2022

Extend PyUnicode_FromFormat() #98836

Closed

serhiy-storchaka marked this pull request as ready for review

November 3, 2022 15:23

serhiy-storchaka requested review from lysnikolaou and pablogsal as code owners

November 3, 2022 15:23

serhiy-storchaka requested review from vstinner and removed request for lysnikolaou and pablogsal

November 3, 2022 18:02

vstinner reviewed

Nov 4, 2022

View reviewed changes

Comment thread

Doc/whatsnew/3.12.rst Outdated

		:c:func:`PyUnicode_FromFormatV` now sets a :exc:`SystemError`.
		In previous versions it caused all the rest of the format string to be
		copied as-is to the result string, and any extra arguments discarded.
		(Contributed by Serhiy Storchaka in :gh:`95781`.)

vstinner Nov 4, 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 seems like this paragraph was duplicated by mistake?

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka Nov 6, 2022

Copy link

Copy Markdown

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

True.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_unicode.py Outdated

		c_int, c_long, c_longlong, c_ssize_t,
		c_uint, c_ulong, c_ulonglong, c_size_t, c_void_p)
		c_uint, c_ulong, c_ulonglong, c_size_t, c_void_p,
		sizeof, c_wchar, c_wchar_p)

vstinner Nov 4, 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.

Can you please a comment somewhere to list formats which are not tested and the reason why?

I see at least %j and %t which are not tested. Please mention that _testcapi.test_string_from_format() has a wider coverage of all formats, and test these formats.

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka Nov 6, 2022

Copy link

Copy Markdown

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

Done.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_unicode.py Outdated

		check_format(' 0000123', b'%10.7i', c_int(123))
		check_format('0000000123', b'%010.7i', c_int(123))
		check_format('0000123 ', b'%-10.7i', c_int(123))
		check_format('0000123 ', b'%-010.7i', c_int(123))

vstinner Nov 4, 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.

Would it be possible to generate these tests? The code is similar for the 6 groups of tests.

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka Nov 6, 2022

Copy link

Copy Markdown

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

It is not so similar. It is different for signed and unsigned types, but I'll try to generalize it.

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner Nov 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.

I just suggested to generate these tests. It's ok to leave them as they are if it's too complicated to generate them.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_unicode.py Outdated

		b'%V', None, b'xyz')

		# test %ls
		check_format('abc', b'%ls', c_wchar_p('abc'))

vstinner Nov 4, 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.

You can please add one non-ASCII character in wchar tests, %ls and %lV? In addition to tests truncating to 2 wchar_t.

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka Nov 6, 2022

Copy link

Copy Markdown

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

What do you mean? Isn't the following line enough?

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner Nov 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.

I'm thinking at adding a test with non-ASCII characters which fit into UCS-2 (16-bit wchar_t) characters. Something like:

check_format('a\xe9\u20ac', b'%ls', c_wchar_p('a\xe9\u20ac'))

Some bugs are only triggered depending on the maximum code point, and wchar_t code can take different code path depending if there is a surrogate character or not.

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka Nov 20, 2022

Copy link

Copy Markdown

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

Done.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_unicode.py Outdated

		@@ -2880,10 +2989,11 @@ def check_format(expected, format, *args):
		# check for crashes

vstinner Nov 4, 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.

this comment is misleading. Python doesn't crash but reject invalid format string with a SyntaxError. Would you mind to rephrase the comment? Like: # test invalid format strings?

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka Nov 6, 2022

Copy link

Copy Markdown

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

Done. Restored the part of the older comment.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Doc/c-api/unicode.rst

		\| \| \| :c:func:`PyObject_Repr`. \|
		+-------------------+---------------------+----------------------------------+
		ASCII-encoded string.

vstinner Nov 4, 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.

Can you please add something like [[fill]align][sign][z][#][0][width][grouping_option][.precision][type]? Example taken from: https://docs.python.org/dev/library/string.html#format-specification-mini-language

The printf format is complex, and it's not obvious in which order each part should be written. I understand that it's something like:

%[conversion flags][minimum width][.precision][length modifier][conversion type]

Or if you prefer a shorter version:

%[conversion][width][.precision][modifier][conversion]

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka Nov 6, 2022

Copy link

Copy Markdown

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

I copied the following paragraphs from the description of the print-like formatting in Python: https://docs.python.org/3/library/stdtypes.html#printf-style-string-formatting.

I think that it also can benefit from from similar changes, but let leave it to other issue.

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka added 5 commits

November 6, 2022 19:35


          Merge branch 'main' into capi-unicode-fromformat

8942ca3


          Remove duplicated paragraph.

8481bda


          Address review comments.


          Fix syntax error in a comment.

9ffdb86


          Fix and refactor tests.

9ca3764

vstinner approved these changes

Nov 7, 2022

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_unicode.py Outdated

		check_format(' 0000123', b'%10.7i', c_int(123))
		check_format('0000000123', b'%010.7i', c_int(123))
		check_format('0000123 ', b'%-10.7i', c_int(123))
		check_format('0000123 ', b'%-010.7i', c_int(123))

vstinner Nov 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.

I just suggested to generate these tests. It's ok to leave them as they are if it's too complicated to generate them.

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_unicode.py Outdated

		# Length modifiers "j" and "t" are not tested here because ctypes does
		# not expose types for intmax_t and ptrdiff_t.
		# _testcapi.test_string_from_format() has a wider coverage of all
		# formats.

vstinner Nov 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.

Thank you, this comment is useful to me at least :-)

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_unicode.py Outdated

		b'%V', None, b'xyz')

		# test %ls
		check_format('abc', b'%ls', c_wchar_p('abc'))

vstinner Nov 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.

I'm thinking at adding a test with non-ASCII characters which fit into UCS-2 (16-bit wchar_t) characters. Something like:

check_format('a\xe9\u20ac', b'%ls', c_wchar_p('a\xe9\u20ac'))

Some bugs are only triggered depending on the maximum code point, and wchar_t code can take different code path depending if there is a surrogate character or not.

Sorry, something went wrong.

Uh oh!

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

All reactions

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

Nov 7, 2022

vstinner commented Nov 7, 2022

Copy link

Copy Markdown

Member

Feel free to ignore my suggestions, the PR LGTM.

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka added 2 commits

November 20, 2022 11:38


          Merge branch 'main' into capi-unicode-fromformat

79a523c


          Add more tests for %ls and %lV.

91d8393

vstinner approved these changes

Nov 21, 2022

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

serhiy-storchaka added 2 commits

May 21, 2023 23:11


          Merge branch 'main' into capi-unicode-fromformat

9f2f5f2


          Try to silence Sphinx warnings.

b86d92d

serhiy-storchaka merged commit f3466bc into python:main

May 21, 2023

bedevere-bot removed the awaiting merge label

May 21, 2023

serhiy-storchaka deleted the capi-unicode-fromformat branch

May 21, 2023 21:32

Reviewers

vstinner

vstinner approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

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

Conversation

serhiy-storchaka commented Oct 29, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Nov 7, 2022

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

serhiy-storchaka commented Oct 29, 2022 •

edited by bedevere-bot

Loading

Uh oh!

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