| msg149420 - (view) |
Author: Ramchandra Apte (Ramchandra Apte) * |
Date: 2011-12-14 03:59 |
| string.Formatter doesn't support empty curly braces "{}" unlike str.format . >>> import string >>> a = string.Formatter() >>> a.format("{}","test") Traceback (most recent call last): File "<pyshell#2>", line 1, in a.format("{}","hello") File "/usr/lib/python3.2/string.py", line 180, in format return self.vformat(format_string, args, kwargs) File "/usr/lib/python3.2/string.py", line 184, in vformat result = self._vformat(format_string, args, kwargs, used_args, 2) File "/usr/lib/python3.2/string.py", line 206, in _vformat obj, arg_used = self.get_field(field_name, args, kwargs) File "/usr/lib/python3.2/string.py", line 267, in get_field obj = self.get_value(first, args, kwargs) File "/usr/lib/python3.2/string.py", line 226, in get_value return kwargs[key] KeyError: '' |
|
|
| msg149424 - (view) |
Author: Ramchandra Apte (Ramchandra Apte) * |
Date: 2011-12-14 05:39 |
| Attached is patch to fix this issue. |
|
|
| msg149426 - (view) |
Author: Ramchandra Apte (Ramchandra Apte) * |
Date: 2011-12-14 05:51 |
| Sorry, the patch has an mistake. ValueErro should be ValueError. |
|
|
| msg149433 - (view) |
Author: Ramchandra Apte (Ramchandra Apte) * |
Date: 2011-12-14 09:06 |
| Attached is a patch for test.test_string to test for this bug. Can somebody please comment on my paches or commit my patches. |
|
|
| msg149515 - (view) |
Author: Ramchandra Apte (Ramchandra Apte) * |
Date: 2011-12-15 07:42 |
| Why isn't anybody commiting or commenting on my patches? |
|
|
| msg149523 - (view) |
Author: Eric V. Smith (eric.smith) *  |
Date: 2011-12-15 10:15 |
| This bug is assigned to me. Sometimes it takes a while before a committer has time to review a bug and act on it. I can assure you that I will review this before the next release of Python. Thank you for the bug report, and especially thanks for the patch! One thing that will definitely need to be developed before this is committed is one or more tests. Either you can add them, or I will before I commit the fix. There are some existing tests for str.format that can be leveraged for string.Formatter. |
|
|
| msg149566 - (view) |
Author: Meador Inge (meador.inge) * |
Date: 2011-12-15 17:26 |
| On Thu, Dec 15, 2011 at 1:42 AM, maniram maniram <report@bugs.python.org> wrote: > Why isn't anybody commiting or commenting on my patches? Your patch is much appreciated. Thank you. It takes some time to get things reviewed. Please read the Dev Guide. In particular, the "Lifecycle of a Patch" section: http://docs.python.org/devguide/patch.html. Also, please quit marking issues as "languishing" unless the issue meets the qualifications spelled out here: http://docs.python.org/devguide/languishing.html. |
|
|
| msg150507 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2012-01-03 16:16 |
| test_string.diff looks good, except that it should probably only test the exception type, not the message (they are not a guaranteed part of the Python language and may change arbitrarily between versions or implementations (e.g. PyPy), so better not to add tests that depend on exact words). I don’t have anything specific to say about .diff; if it makes the test pass, then it’s good. “if manual == True” should just be replaced by “if manual”. If you’d like to, you can make one patch with fix + tests that addresses my comments and remove the older diffs. |
|
|
| msg153397 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2012-02-15 09:01 |
| One potential problem with the simple approach to fixing this is that up until now, string.Formatter has been thread safe. Because all the formatting state was held in local variables and passed around as method arguments, there was no state on the instance object to protect. Now, this only applies if you start using the new feature, but it should be noted in the documentation and What's New that you need to limit yourself to accessing each formatter instance from a single thread. It's also enough for me to say "no, not in a maintenance release". Adding two attributes also seems unnecessary, and the pre-increment looks strange. Why not: In __init__: auto_field_count = 0 Then as the auto numbering checking, something like: auto_field_count = self.auto_field_count if field_name: if auto_field_count > 0: # Can't switch auto -> manual auto_field_count = -1 elif auto_field_count < 0: # Can't switch manual -> auto else: field_name = str(auto_field_count) self.auto_field_count += 1 (Alternatively, I'd ask the question: why do we prevent mixing manual numbering and explicit numbering anyway? It's not like it's ambiguous at all) |
|
|
| msg153532 - (view) |
Author: Ramchandra Apte (Ramchandra Apte) * |
Date: 2012-02-17 04:07 |
| @Nick I don't understand why should my patch make Formatter thread-unsafe - the auto_field_count and manual variables are local variables just like the variables in the other functions in Formatter. |
|
|
| msg153533 - (view) |
Author: Ramchandra Apte (Ramchandra Apte) * |
Date: 2012-02-17 04:31 |
| I have submitted a new patch, I have moved the increment to the end of if loop. |
|
|
| msg162207 - (view) |
Author: Ramchandra Apte (Ramchandra Apte) * |
Date: 2012-06-03 12:06 |
| What is the status of the bug? |
|
|
| msg162224 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2012-06-03 17:06 |
| Could you upload just one patch with fix and test, addressing my previous comments, and remove the old patches? It will make it easier for Eric to review when he gets some time. Please also keep lines under 80 characters. Thanks in advance. |
|
|
| msg162226 - (view) |
Author: Vinay Sajip (vinay.sajip) * |
Date: 2012-06-03 18:28 |
| It seems like the patch doesn't consider mixing of positional and keyword arguments: if you have the format string "{foo} {} {bar}", then manual will be set to True when "foo" is seen as the field_name, and fail soon after when "" is seen as the field_name the next time around. So, the test should include something which shows that fmt.format("{foo} {} {bar}", 2, foo='fooval', bar='barval') returns "fooval 2 barval", whereas with a format string like "{foo} {0} {} {bar}" or "{foo} {} {0} {bar}" you get a ValueError. Also, why "automatic field numbering" vs. "manual field specification"? Why not "numbering" for both? |
|
|
| msg162254 - (view) |
Author: Ramchandra Apte (Ramchandra Apte) * |
Date: 2012-06-04 10:27 |
| Added a new patch which addresses Éric's comments. |
|
|
| msg162549 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2012-06-08 22:02 |
| Is there a reason "manual" is None, True, or False? Wouldn't just True or False suffice? |
|
|
| msg162550 - (view) |
Author: Vinay Sajip (vinay.sajip) * |
Date: 2012-06-08 22:16 |
| > Is there a reason "manual" is None, True, or False? Wouldn't just True or False suffice? I suppose before we see the first bracketed form ({} or {\d+}) we don't know which it is. |
|
|
| msg162551 - (view) |
Author: Eric V. Smith (eric.smith) * |
Date: 2012-06-08 22:49 |
| Yes, I guess that's so. I'll have to add a comment, as at first glance it just looks like a bug. Thanks! |
|
|
| msg162562 - (view) |
Author: Ramchandra Apte (Ramchandra Apte) * |
Date: 2012-06-09 14:51 |
| Its not a bug though it has maintenance problems because if you change "manual is False" to not manual it no longer works correctly. |
|
|
| msg162565 - (view) |
Author: Vinay Sajip (vinay.sajip) * |
Date: 2012-06-09 21:26 |
| > it has maintenance problems because if you change "manual is False" > to not manual it no longer works correctly. So you should probably comment the initialisation appropriately. |
|
|
| msg162668 - (view) |
Author: Alyssa Coghlan (ncoghlan) * |
Date: 2012-06-12 11:57 |
| One brief comment on the wording of the error message: the inconsistent naming is actually copied from the str.format code. >>> "{foo} {} {bar}".format(2, foo='fooval', bar='barval') 'fooval 2 barval' >>> "{foo} {0} {} {bar}".format(2, foo='fooval', bar='barval') Traceback (most recent call last): File "", line 1, in ValueError: cannot switch from manual field specification to automatic field numbering |
|
|
| msg174272 - (view) |
Author: Phil Elson (pelson) * |
Date: 2012-10-31 12:08 |
| The current patch fails to catch the fact that auto vs manual numbering has been used in following corner case: from string import Formatter print(Formatter().format("{0:{}}", 'foo', 5)) To fix this, without adding state to the formatter instance, some more information is going to need to be passed to the _vformat method. |
|
|
| msg174282 - (view) |
Author: Phil Elson (pelson) * |
Date: 2012-10-31 14:22 |
| Ramchandra's fix looks fairly good, although there is at least one remaining issue (see my last comment). I have attached a patch which addresses (and tests) this. I'd be happy to pick this up if there are any remaining issues that need to be addressed, otherwise: I hope this helps. Thanks, |
|
|
| msg180281 - (view) |
Author: Ramchandra Apte (Ramchandra Apte) * |
Date: 2013-01-20 05:43 |
| Buump. |
|
|
| msg180290 - (view) |
Author: Vinay Sajip (vinay.sajip) * |
Date: 2013-01-20 13:03 |
| Looking at Phil Elson's patch: I didn't see a test case relating to the example in his comment, namely f.format("{0:{}}", 'foo', 5) Did I miss it? The documentation also needs to be updated. |
|
|
| msg183434 - (view) |
Author: Phil Elson (pelson) * |
Date: 2013-03-04 12:09 |
| > I didn't see a test case relating to the example in his comment, namely > > f.format("{0:{}}", 'foo', 5) > > Did I miss it? The example should fail, which it wouldn't have done with the patch previously proposed. I believe the case is covered by the block: with self.assertRaises(ValueError): fmt.format("foo{1}{}", "bar", 6) Though there is no harm in adding another test along the lines of: with self.assertRaises(ValueError): fmt.format("{0:{}}", "bar", 6) If you think it is worthwhile? I'm uncertain which documentation to update since the method which has had its signature updated is private and is called solely by Formatter.vformat . Cheers, |
|
|
| msg183453 - (view) |
Author: Vinay Sajip (vinay.sajip) * |
Date: 2013-03-04 13:48 |
| > I believe the case is covered by the block: [snip] Ah, right. I wasn't sure that was the exact same code path that was being exercised. But I didn't look very closely. > If you think it is worthwhile? Only if it exercises a different code path. |
|
|
| msg216207 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2014-04-14 20:44 |
| New changeset ad74229a6fba by Eric V. Smith in branch '3.4': Issue #13598: Add auto-numbering of replacement fields to string.Formatter. http://hg.python.org/cpython/rev/ad74229a6fba |
|
|
| msg216208 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2014-04-14 20:47 |
| New changeset 50fe497983fd by Eric V. Smith in branch '3.4': Issue #13598: Added acknowledgements to Misc/NEWS. http://hg.python.org/cpython/rev/50fe497983fd |
|
|
| msg218114 - (view) |
Author: Armin Ronacher (aronacher) * |
Date: 2014-05-08 14:46 |
| Is there any chance this will be fixed for 2.7 as well? |
|
|