| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
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 usernameWe couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames: 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! |
Sorry, something went wrong.
|
Blurb is trivial, but just in case the following one can be used:
|
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
That was my starting point as well. I have a use case in mind for which these changes don't cut it. 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. |
Sorry, something went wrong.
I think this is great. |
Sorry, something went wrong.
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). |
Sorry, something went wrong.
Good point. t-strings will (hopefully) be great. Unfortunately, it will be a while before I will be able to use them. In any case, I think this specific PR is useful. From my POV it fixes a bug. See: #129273 (comment) |
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
(Apologies for rebasing in the same change as fixing the issue, here is the relevant change.) |
Sorry, something went wrong.
|
No problem; thank you for the contribution! |
Sorry, something went wrong.
|
@encukou |
Sorry, something went wrong.
|
The reproducer in #89867 still behaves as before. I don't currently have time to look into the details there. |
Sorry, something went wrong.
|
I'll treat this as a feature and not backport. Let me know if you want to reconsider. |
Sorry, something went wrong.
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. The reason for it is clear and unambiguous. |
Sorry, something went wrong.
Fine by me. Thank you for quick turnaround! |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
https://bugs.python.org/issue27307