Merge lp:~mnordhoff/bzr/st-size-exceptions into lp:bzr

[Matt Nordhoff]

[Matt Nordhoff (mnordhoff)]

This makes the changes I suggested in bug #457979:

> Lookie:
>
> if (size < 0) {
> PyErr_BadInternalCall();
> return NULL;
> }
>
> if (size < 0 || size > 255) {
> /* Too big or too small */
> PyErr_SetString(PyExc_ValueError, "StaticTuple(...)"
> " takes from 0 to 255 items");
> return NULL;
> }
>
> Not only is this redundant, but PyErr_BadInternalCall() throws a
> TypeError, meaning two different exception classes are used.
>
> ISTM StaticTuple_New should throw a ValueError, since you do
> StaticTuple_New(length), and StaticTuple_new_constructor should throw a
> TypeError, since you do StaticTuple(foo, bar, baz) (currently it lets
> StaticTuple_New handle throw the error).
>
> FYI, the Python version of StaticTuple throws a ValueError when it's
> either <0 or >255. IMO, that should be a TypeError, same as
> StaticTuple_new_constructor.

* bzrlib/_static_tuple_c.c:
(StaticTuple_New): Raise a ValueError when size <0, just like when it's >255.
(StaticTuple_new_constructor): Raise a TypeError when this happens, instead of letting StaticTuple_New raise its ValueError.

* bzrlib/_static_tuple_py.py:
(StaticTuple.__init__): Change the ValueError to a TypeError as well. Move the length check before the type checks, to match StaticTuple_new_constructor, and copy over its error message, since I like it better and it doesn't refer to "key bits".

* bzrlib/tests/test__static_tuple.py:
(TestStaticTuple.test_create_bad_args): Make the relevant type adjustments.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Matt Nordhoff wrote:
> Matt Nordhoff has proposed merging lp:~mnordhoff/bzr/st-size-exceptions into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This makes the changes I suggested in bug #457979:
>
>> Lookie:
>>
>> if (size < 0) {
>> PyErr_BadInternalCall();
>> return NULL;
>> }
>>
>> if (size < 0 || size > 255) {
>> /* Too big or too small */
>> PyErr_SetString(PyExc_ValueError, "StaticTuple(...)"
>> " takes from 0 to 255 items");
>> return NULL;
>> }
>>
>> Not only is this redundant, but PyErr_BadInternalCall() throws a
>> TypeError, meaning two different exception classes are used.
>>
>> ISTM StaticTuple_New should throw a ValueError, since you do
>> StaticTuple_New(length), and StaticTuple_new_constructor should throw a
>> TypeError, since you do StaticTuple(foo, bar, baz) (currently it lets
>> StaticTuple_New handle throw the error).
>>
>> FYI, the Python version of StaticTuple throws a ValueError when it's
>> either <0 or >255. IMO, that should be a TypeError, same as
>> StaticTuple_new_constructor.
>
> * bzrlib/_static_tuple_c.c:
> (StaticTuple_New): Raise a ValueError when size <0, just like when it's >255.
> (StaticTuple_new_constructor): Raise a TypeError when this happens, instead of letting StaticTuple_New raise its ValueError.
>
> * bzrlib/_static_tuple_py.py:
> (StaticTuple.__init__): Change the ValueError to a TypeError as well. Move the length check before the type checks, to match StaticTuple_new_constructor, and copy over its error message, since I like it better and it doesn't refer to "key bits".
>
> * bzrlib/tests/test__static_tuple.py:
> (TestStaticTuple.test_create_bad_args): Make the relevant type adjustments.
>

I'm happy to have them the same. Regardless of what we make this, it
isn't something you should ever be catching, so the specific exception
isn't particularly meaningful.

It is arguable that "TypeError" should be reserved for when the *type*
of the argument passed is invalid. (Such as passing a dict rather than a
string/unicode/etc.)

Everything else would be considered a ValueError. (Having a tuple that
holds only strings but is 266 entries long is closer to a ValueError
than a TypeError.)

John
=:->

 review needs_fixing
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrgdD4ACgkQJdeBCYSNAAPlJwCePnGWlGEZapi4rhFv6le6DrYq
cIUAoJtXJiAbJ0mszyScsrMm38vcwCHE
=0f8p
-----END PGP SIGNATURE-----

review: Needs Fixing

[Martin Pool (mbp)]

John you voted 'needs fixing' but I'm not clear what fixes you want to see.

[John A Meinel (jameinel)]

I think we had talked about it a while on IRC, and it seems that invalid arguments are pretty much always a type error. So we can go ahead and merge it as is.

review: Approve

=== modified file 'bzrlib/_static_tuple_c.c'
--- bzrlib/_static_tuple_c.c	2009-10-27 14:07:16 +0000
+++ bzrlib/_static_tuple_c.c	2010-02-17 20:49:16 +0000
@@ -140,10 +140,6 @@
 StaticTuple_New(Py_ssize_t size)
 {
     StaticTuple *stuple;
-    if (size < 0) {
-        PyErr_BadInternalCall();
-        return NULL;
-    }
 
     if (size < 0 || size > 255) {
         /* Too big or too small */
@@ -280,6 +276,14 @@
         return NULL;
     }
     len = PyTuple_GET_SIZE(args);
+    if (len < 0 || len > 255) {
+        /* Check the length here so we can raise a TypeError instead of
+         * StaticTuple_New's ValueError.
+         */
+        PyErr_SetString(PyExc_TypeError, "StaticTuple(...)"
+            " takes from 0 to 255 items");
+        return NULL;
+    }
     self = (StaticTuple *)StaticTuple_New(len);
     if (self == NULL) {
         return NULL;

=== modified file 'bzrlib/_static_tuple_py.py'
--- bzrlib/_static_tuple_py.py	2009-11-28 21:54:08 +0000
+++ bzrlib/_static_tuple_py.py	2010-02-17 20:49:16 +0000
@@ -34,15 +34,15 @@
 
     def __init__(self, *args):
         """Create a new 'StaticTuple'"""
+        num_keys = len(args)
+        if num_keys < 0 or num_keys > 255:
+            raise TypeError('StaticTuple(...) takes from 0 to 255 items')
         for bit in args:
             if type(bit) not in (str, StaticTuple, unicode, int, long, float,
                                  None.__class__, bool):
                 raise TypeError('StaticTuple can only point to'
                     ' StaticTuple, str, unicode, int, long, float, bool, or'
                     ' None not %s' % (type(bit),))
-        num_keys = len(args)
-        if num_keys < 0 or num_keys > 255:
-            raise ValueError('must have 1 => 256 key bits')
         # We don't need to pass args to tuple.__init__, because that was
         # already handled in __new__.
         tuple.__init__(self)

=== modified file 'bzrlib/tests/test__static_tuple.py'
--- bzrlib/tests/test__static_tuple.py	2009-12-22 16:28:47 +0000
+++ bzrlib/tests/test__static_tuple.py	2010-02-17 20:49:16 +0000
@@ -77,9 +77,9 @@
     def test_create_bad_args(self):
         args_256 = ['a']*256
         # too many args
-        self.assertRaises(ValueError, self.module.StaticTuple, *args_256)
+        self.assertRaises(TypeError, self.module.StaticTuple, *args_256)
         args_300 = ['a']*300
-        self.assertRaises(ValueError, self.module.StaticTuple, *args_300)
+        self.assertRaises(TypeError, self.module.StaticTuple, *args_300)
         # not a string
         self.assertRaises(TypeError, self.module.StaticTuple, object())