Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-71494: string.Formatter unnumbered key/attributes #21767

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

qm2k
Copy link
Contributor

@qm2k qm2k commented Aug 7, 2020

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@qm2k

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

@qm2k
Copy link
Contributor Author

qm2k commented Aug 7, 2020

Blurb is trivial, but just in case the following one can be used:

Add support for format strings like '{.name}' and '{[1]}' already supported by str.format to string.Formatter

@qm2k
Copy link
Contributor Author

qm2k commented Aug 11, 2020

Rationale: we need to preserve the reenterability (which I think is important, e.g. in the looging use cases), and also not break the existing user subclasses that expect '' to be converted to '0' (etc.) before the get_field(...) call. Cases where field_name starts with an open bracket or dot would still change behaviour, but there's nothing we can do about it since original base class implementation raised exception on them and that is exactly what we are trying to fix.

The alternative would be to add the missing reference in the get_field(...) itself, but it's hard to do it there since this function doesn't receive the numbering source, and adding new argument with it would break existing user subclasses. Passing the numbering source in a class attribute would break reenterability. It is possible to use some attribute with a stack of thread-local/traceback-keyed numbering sources to preserve reenterability and (some) thread-safety, but it would be hugely counterintuitive and likely quite fragile. Besides, changing the default get_field(...) implementation would not fix existing user subclasses that redefine this function. Therefore the only way to go was to fix the field_name before the get_field(...) call, which is what I did.

@qm2k qm2k force-pushed the string_formatter_auto_numbering_lookup branch from 638aceb to 3a4440e Compare April 14, 2024 18:13
Copy link

cpython-cla-bot bot commented Apr 14, 2024

All commit authors signed the Contributor License Agreement.
CLA signed

@qm2k qm2k force-pushed the string_formatter_auto_numbering_lookup branch from 3a4440e to bfef488 Compare April 14, 2024 18:20
@qm2k qm2k force-pushed the string_formatter_auto_numbering_lookup branch from bfef488 to 7d1522e Compare April 28, 2024 17:53
Lib/string.py Outdated Show resolved Hide resolved
Lib/string.py Outdated Show resolved Hide resolved
Lib/string.py Outdated Show resolved Hide resolved
Lib/string.py Outdated Show resolved Hide resolved
Lib/string.py Outdated Show resolved Hide resolved
@dg-pb
Copy link
Contributor

dg-pb commented Jan 25, 2025

I see now that you have copied AutoNumber concept from C.

Personally, I like your changes, but for this to go through it would probably be best to only leave what is necessary. Which to me seems to be limited to auto_reference body, which can be placed in the main loop.

@qm2k qm2k force-pushed the string_formatter_auto_numbering_lookup branch from c4b847d to 4bb189e Compare January 25, 2025 22:30
@qm2k
Copy link
Contributor Author

qm2k commented Jan 25, 2025

Personally, I like your changes, but for this to go through it would probably be best to only leave what is necessary. Which to me seems to be limited to auto_reference body, which can be placed in the main loop.

I am starting to remember my motivation for separate class and method: it was to provide access to corresponding parts to the user for further customization. I think nobody uses string.Formatter as it is, since by itself it is no better than str.format; most users probably subclass it and override some methods. I cannot however remember if my specific use case required further customization of automatic numbering or not.

I went ahead and made the change absolutely minimal which should address all of the issues raised in your comments. If somebody needs additional entry points for customization they are always welcome to submit a pull request :))) Please tell if you like the new version better.

@dg-pb
Copy link
Contributor

dg-pb commented Jan 25, 2025

I am starting to remember my motivation for separate class and method

That was my starting point as well.
I wanted to pass auto_arg_index to get_field.
Issue is that it is very hard to do something without breaking backwards compatibility.

I have a use case in mind for which these changes don't cut it.
Namely, PartialFormatter, which needs get_field to have both original field_name and also auto_arg_index adjustments.

Then I thought of something along the lines of AutoNumber to pass it easily to methods, so initially it made sense, but eventually realised that this path is a bit tricky.

Long story short, I will spend some more time, maybe there is some good path forward. If there is not, simply pushing minimal bug fix is probably better than nothing at all.

@dg-pb
Copy link
Contributor

dg-pb commented Jan 25, 2025

Please tell if you like the new version better.

I think this is great.
This will fix the issue at hand with minimal changes.
And yes, agreed, other things are best left to be done in separate PRs.

Lib/string.py Outdated Show resolved Hide resolved
@qm2k qm2k force-pushed the string_formatter_auto_numbering_lookup branch from 4bb189e to 806199e Compare January 26, 2025 09:40
@qm2k
Copy link
Contributor Author

qm2k commented Jan 26, 2025

I am starting to remember my motivation for separate class and method

That was my starting point as well. I wanted to pass auto_arg_index to get_field. Issue is that it is very hard to do something without breaking backwards compatibility.

I have a use case in mind for which these changes don't cut it. Namely, PartialFormatter, which needs get_field to have both original field_name and also auto_arg_index adjustments.

Then I thought of something along the lines of AutoNumber to pass it easily to methods, so initially it made sense, but eventually realised that this path is a bit tricky.

Long story short, I will spend some more time, maybe there is some good path forward. If there is not, simply pushing minimal bug fix is probably better than nothing at all.

Have you seen PEP750? I feel like any new work on custom formatters should make use of t-strings (but interestingly some examples there still subclass good old string.Formatter).

@qm2k qm2k force-pushed the string_formatter_auto_numbering_lookup branch from 806199e to 068deb5 Compare January 26, 2025 10:33
@dg-pb
Copy link
Contributor

dg-pb commented Jan 26, 2025

Have you seen PEP750? I feel like any new work on custom formatters should make use of t-strings (but interestingly some examples there still subclass good old string.Formatter).

Good point.

t-strings will (hopefully) be great. Unfortunately, it will be a while before I will be able to use them.
t-strings is still in draft - it might take some time before it is out.
I am not even sure if it is final concept.
It had at least 1 radical change in the course of last few (6 maybe) months.
I am now on 3.12 and still keep compatibility to 3.8.
Thus, I think string.Formatter is still relevant.

In any case, I think this specific PR is useful. From my POV it fixes a bug. See: #129273 (comment)

Copy link
Contributor

@dg-pb dg-pb left a comment

Choose a reason for hiding this comment

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

LGTM

Lib/test/test_string.py Outdated Show resolved Hide resolved
@dg-pb
Copy link
Contributor

dg-pb commented Jan 27, 2025

I think this is worth mentioning.

_string.formatter_field_name_split doesn't parse the whole string on call (returns Iterator for 2nd value), thus although it might seem that calling it twice could result in performance penalty, in reality it has negligible impact.

Lib/test/test_string.py Outdated Show resolved Hide resolved
@encukou encukou changed the title bpo-27307: string.Formatter unnumbered key/attributes gh-71494: string.Formatter unnumbered key/attributes Jan 27, 2025
@qm2k qm2k force-pushed the string_formatter_auto_numbering_lookup branch from 1e55873 to a493f9c Compare January 27, 2025 21:10
@qm2k
Copy link
Contributor Author

qm2k commented Jan 27, 2025

(Apologies for rebasing in the same change as fixing the issue, here is the relevant change.)

@encukou encukou merged commit 22b2d37 into python:main Jan 31, 2025
38 checks passed
@encukou
Copy link
Member

encukou commented Jan 31, 2025

No problem; thank you for the contribution!

@encukou
Copy link
Member

encukou commented Jan 31, 2025

The reproducer in #89867 still behaves as before. I don't currently have time to look into the details there.

@encukou
Copy link
Member

encukou commented Jan 31, 2025

I'll treat this as a feature and not backport. Let me know if you want to reconsider.
(Backporting would mean that code tested on 3.13.2 could fail on 3.13.1.)

@dg-pb
Copy link
Contributor

dg-pb commented Jan 31, 2025

The reproducer in #89867 still behaves as before. I don't currently have time to look into the details there.

You are right, it is not the same thing, I misread it.

However, I had a look at it and that request IMO is a no-go.
I have commented on it with suggestion to close.

The reason for it is clear and unambiguous.
If you have few minutes to spare, I think you could close that one as well.

@dg-pb
Copy link
Contributor

dg-pb commented Jan 31, 2025

I'll treat this as a feature and not backport.

Fine by me.

Thank you for quick turnaround!

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants