R. David Murray added the comment:
I agree with Chris here. We also need to add the various tests he's come up
with. IMO the easiest way to restore the original behavior and the preserve
the fixes is to make the following single line change:
diff --git a/Lib/argparse.py b/Lib/argparse.py
Steven Bethard added the comment:
It looks like the correct fix was already applied, but just to chime in here:
(1) Yes, the error is that the isinstance(action.default, str) check was lost
(2) Yes, it is intended that you can use a string value as your default and the
type= converter will be
R. David Murray added the comment:
The correct fix has not been applied yet.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
___
___
Steven Bethard added the comment:
Oh, I see, you're right - the recent changes from the Roundup Robot are exactly
the wrong changes - special casing _StoreAction, not string defaults.
--
___
Python tracker rep...@bugs.python.org
Barry A. Warsaw added the comment:
On Sep 12, 2012, at 10:57 AM, Steven Bethard wrote:
(1) Yes, the error is that the isinstance(action.default, str) check was lost
Except that it won't work any more in the original location. I tried it and
it broke other tests. Maybe I did it wrong, so
Barry A. Warsaw added the comment:
On Sep 12, 2012, at 10:39 AM, R. David Murray wrote:
diff --git a/Lib/argparse.py b/Lib/argparse.py
--- a/Lib/argparse.py
+++ b/Lib/argparse.py
@@ -1962,7 +1962,8 @@
# only if it was defined already in the namespace
if
Barry A. Warsaw added the comment:
On Sep 12, 2012, at 04:22 AM, Chris Jerdonek wrote:
The argparse documentation makes it pretty clear that 'type' is meant to be
applied only to strings.
Then test_type_function_call_with_non_string_default() which was added to fix
#12776 and #11839 is a bogus
Steven Bethard added the comment:
I see. So right now, both string defaults and non-string defaults are being
converted with the type= function. That seems suspect to me since the
documentation explicitly says type= can take any callable that takes a single
string argument and returns the
R. David Murray added the comment:
*No* code should depend on it: this fix is very recent and is not in any
released version of Python, not even the RCs.
The bogus test should be removed. When I committed that patch I did not
understand the correct (documented) semantics of default
R. David Murray added the comment:
Actually, no, the test should not be removed, it should be reversed so as to
test the documented behavior.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
Barry A. Warsaw added the comment:
On Sep 12, 2012, at 03:03 PM, R. David Murray wrote:
Actually, no, the test should not be removed, it should be reversed so as to
test the documented behavior.
Good point.
--
___
Python tracker
Steven Bethard added the comment:
Ok, sounds good. Let's make the test check the documented behavior, and then
add back the isinstance(action.default, str) check.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
Changes by Barry A. Warsaw ba...@python.org:
Added file: http://bugs.python.org/file27180/15906-3.diff
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
___
Barry A. Warsaw added the comment:
On Sep 12, 2012, at 02:48 PM, Steven Bethard wrote:
We should not be converting non-string defaults, or the documentation's
description of the type= argument doesn't make sense.
Agreed. If we also take RDM's suggestion of reversing the sense of
Barry A. Warsaw added the comment:
On Sep 12, 2012, at 03:40 PM, Steven Bethard wrote:
Ok, sounds good. Let's make the test check the documented behavior, and then
add back the isinstance(action.default, str) check.
See patch 15906-3.diff for Python 2.7. If acceptable, I will apply and
R. David Murray added the comment:
Looks good to me.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
___
___
Python-bugs-list mailing list
Barry A. Warsaw added the comment:
Oops, one small correction for 15906-3.diff: In Python 2.7, s/str/basestring/
Obviously isinstance should just check for str-y-ness in Python 3.{2,3}
--
___
Python tracker rep...@bugs.python.org
Steven Bethard added the comment:
Patch, with the basestring amendment, looks good.
Do we need a new test for conversion of string defaults?
Yeah, I guess go ahead and add one. That will at least document our intentions
here, and if we decide to change that later, then it will force us to
Chris Jerdonek added the comment:
Do we need a new test for conversion of string defaults?
I think we should also update the documentation of the default keyword
argument (and/or the type argument) to remove any ambiguity and make the
behavior more clear.
Maybe one way to view what is
R. David Murray added the comment:
All three of those sound like good ideas (testing string conversion, clarifying
docs, adding the no-double conversion test). Do you want to prepare the patch,
Chris? Barry can apply his any time and yours can be a followup.
We should also make a doc update
Chris Jerdonek added the comment:
Do you want to prepare the patch, Chris?
Sure, I should be able to get to this today or tomorrow.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
Barry A. Warsaw added the comment:
On Sep 12, 2012, at 05:19 PM, R. David Murray wrote:
All three of those sound like good ideas (testing string conversion,
clarifying docs, adding the no-double conversion test). Do you want to
prepare the patch, Chris? Barry can apply his any time and yours
Roundup Robot added the comment:
New changeset 8f847f66a49f by Barry Warsaw in branch '2.7':
A follow up for issue #15906: change the test for calling the type conversion
http://hg.python.org/cpython/rev/8f847f66a49f
New changeset 088b16bd6396 by Barry Warsaw in branch '3.2':
A follow up for
Barry A. Warsaw added the comment:
Chris, it's all yours. I am however going to close the bug as fixed.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
___
Changes by Barry A. Warsaw ba...@python.org:
--
resolution: - fixed
status: open - closed
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
___
Chris Jerdonek added the comment:
Thanks for committing the change. However--
+def test_no_double_type_conversion_of_default(self):
+def extend(str_to_convert):
+return str_to_convert + '*'
+
+parser = argparse.ArgumentParser()
+
Chris Jerdonek added the comment:
Oh, never mind. The initial default value has one star to begin with.
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
___
Chris Jerdonek added the comment:
I think that will just leave the doc updates for Chris once my patch lands in
all three branches.
I created an issue for this here:
http://bugs.python.org/issue15935
--
___
Python tracker rep...@bugs.python.org
Barry A. Warsaw added the comment:
Okay, this bug is clearly caused by the patch applied for issue 12776. Here's
the patch set url: http://hg.python.org/cpython/rev/74f6d87cd471
Now, if you look at this, I think this changes the semantics for non-string
default with a type converter, but the
Chris Jerdonek added the comment:
I'm not sure of all the implications of this, but it seems like this is a
relevant piece of information from the docs:
type= can take any callable that takes a single string argument and returns
the converted value:
(from
Chris Jerdonek added the comment:
Along the lines of my previous comment, I notice that the following str type
check was removed in the patch for issue 12776:
-if isinstance(action.default, str):
-default = self._get_value(action, default)
-
Chris Jerdonek added the comment:
Here is a type of test case that I think should be considered for addition (to
confirm that the code doesn't double-convert strings in at least one case).
Maybe there is already a test case like this:
class MyString(str): pass
def convert(s):
return
R. David Murray added the comment:
I believe you've identified the broken part of the change, Chris. So to
restore previous behavior we need to add that back correctly.
--
nosy: +r.david.murray
___
Python tracker rep...@bugs.python.org
Changes by R. David Murray rdmur...@bitdance.com:
--
priority: high - deferred blocker
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
___
___
Barry A. Warsaw added the comment:
On Sep 11, 2012, at 04:15 PM, Chris Jerdonek wrote:
I haven't thought about this very long, but what would happen if the type
conversion is only called on arguments and default arguments that are
strings, and otherwise left alone?
I thought about that, and
R. David Murray added the comment:
To repeat: there is no change to be made for 3.3. 3.3.0 will go out the door
with the pre-12776 behavior. So any backward compatibility concerns that apply
to 2.7 and 3.2 also apply to 3.3. Thus I suggest we restore the string check,
and consider an
Barry A. Warsaw added the comment:
On Sep 11, 2012, at 07:00 PM, R. David Murray wrote:
To repeat: there is no change to be made for 3.3. 3.3.0 will go out the door
with the pre-12776 behavior. So any backward compatibility concerns that
apply to 2.7 and 3.2 also apply to 3.3. Thus I suggest
Roundup Robot added the comment:
New changeset 739606bdcba4 by Barry Warsaw in branch '2.7':
- Issue #15906: Fix a regression in argparse caused by the preceding change,
http://hg.python.org/cpython/rev/739606bdcba4
New changeset bc342cd7ed96 by Barry Warsaw in branch '3.2':
- Issue #15906: Fix
Chris Jerdonek added the comment:
[From python-dev:
http://mail.python.org/pipermail/python-dev/2012-September/121683.html ]
I've tried the various suggestions out, and I think from a practical point of
view, a fix for the regression in the 2.7, 3.2 and 3.3 branches should be to
apply the one
New submission from Barry A. Warsaw:
Run the following code snippet:
-snip snip-
import argparse
parser = argparse.ArgumentParser()
parser.add_argument(--test, dest=test, type=str,
default=[], action='append')
args = parser.parse_args()
print args.test, repr(args.test)
-snip
Changes by Barry A. Warsaw ba...@python.org:
--
nosy: +benjamin.peterson, georg.brandl
priority: normal - release blocker
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
___
Barry A. Warsaw added the comment:
Marking it as a release blocker and adding 3.3 since 3.3 hg trunk is affected.
--
versions: +Python 3.3
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
Changes by R. David Murray rdmur...@bitdance.com:
--
nosy: +bethard
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
___
___
Python-bugs-list
Georg Brandl added the comment:
I don't see how this affects 3.3: you seem to be saying the behavior is fine
there.
--
versions: -Python 3.3
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
Barry A. Warsaw added the comment:
On Sep 10, 2012, at 05:21 PM, Georg Brandl wrote:
I don't see how this affects 3.3: you seem to be saying the behavior is fine
there.
No, I thought it was because I tested the Ubuntu rc2 version, but the problem
exists in upstream hg 3.3rc2+ head.
--
Changes by Barry A. Warsaw ba...@python.org:
--
versions: +Python 3.3
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
___
___
Python-bugs-list
Georg Brandl added the comment:
But it's not a release blocker for 3.3 then.
--
priority: release blocker - high
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
___
Barry A. Warsaw added the comment:
On Sep 10, 2012, at 05:59 PM, Georg Brandl wrote:
But it's not a release blocker for 3.3 then.
Fair enough!
--
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
Changes by Arfrever Frehtes Taifersar Arahesis arfrever@gmail.com:
--
nosy: +Arfrever
___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue15906
___
49 matches
Mail list logo
