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

Proposed by Matt Nordhoff on 2009-10-22
Status: Merged
Approved by: John A Meinel on 2010-02-17
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~mnordhoff/bzr/st-size-exceptions
Merge into: lp:bzr
Diff against target: 67 lines (+13/-9)
3 files modified
bzrlib/_static_tuple_c.c (+8/-4)
bzrlib/_static_tuple_py.py (+3/-3)
bzrlib/tests/test__static_tuple.py (+2/-2)
To merge this branch: bzr merge lp:~mnordhoff/bzr/st-size-exceptions
Reviewer Review Type Date Requested Status
John A Meinel 2009-10-22 Approve on 2010-02-17
Review via email: mp+13770@code.launchpad.net
To post a comment you must log in.
Matt Nordhoff (mnordhoff) wrote :

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) wrote :

-----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) wrote :

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

John A Meinel (jameinel) wrote :

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

Updating diff...

An updated diff will be available in a few minutes. Reload to see the changes.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/_static_tuple_c.c'
2--- bzrlib/_static_tuple_c.c 2009-10-27 14:07:16 +0000
3+++ bzrlib/_static_tuple_c.c 2010-02-17 20:49:16 +0000
4@@ -140,10 +140,6 @@
5 StaticTuple_New(Py_ssize_t size)
6 {
7 StaticTuple *stuple;
8- if (size < 0) {
9- PyErr_BadInternalCall();
10- return NULL;
11- }
12
13 if (size < 0 || size > 255) {
14 /* Too big or too small */
15@@ -280,6 +276,14 @@
16 return NULL;
17 }
18 len = PyTuple_GET_SIZE(args);
19+ if (len < 0 || len > 255) {
20+ /* Check the length here so we can raise a TypeError instead of
21+ * StaticTuple_New's ValueError.
22+ */
23+ PyErr_SetString(PyExc_TypeError, "StaticTuple(...)"
24+ " takes from 0 to 255 items");
25+ return NULL;
26+ }
27 self = (StaticTuple *)StaticTuple_New(len);
28 if (self == NULL) {
29 return NULL;
30
31=== modified file 'bzrlib/_static_tuple_py.py'
32--- bzrlib/_static_tuple_py.py 2009-11-28 21:54:08 +0000
33+++ bzrlib/_static_tuple_py.py 2010-02-17 20:49:16 +0000
34@@ -34,15 +34,15 @@
35
36 def __init__(self, *args):
37 """Create a new 'StaticTuple'"""
38+ num_keys = len(args)
39+ if num_keys < 0 or num_keys > 255:
40+ raise TypeError('StaticTuple(...) takes from 0 to 255 items')
41 for bit in args:
42 if type(bit) not in (str, StaticTuple, unicode, int, long, float,
43 None.__class__, bool):
44 raise TypeError('StaticTuple can only point to'
45 ' StaticTuple, str, unicode, int, long, float, bool, or'
46 ' None not %s' % (type(bit),))
47- num_keys = len(args)
48- if num_keys < 0 or num_keys > 255:
49- raise ValueError('must have 1 => 256 key bits')
50 # We don't need to pass args to tuple.__init__, because that was
51 # already handled in __new__.
52 tuple.__init__(self)
53
54=== modified file 'bzrlib/tests/test__static_tuple.py'
55--- bzrlib/tests/test__static_tuple.py 2009-12-22 16:28:47 +0000
56+++ bzrlib/tests/test__static_tuple.py 2010-02-17 20:49:16 +0000
57@@ -77,9 +77,9 @@
58 def test_create_bad_args(self):
59 args_256 = ['a']*256
60 # too many args
61- self.assertRaises(ValueError, self.module.StaticTuple, *args_256)
62+ self.assertRaises(TypeError, self.module.StaticTuple, *args_256)
63 args_300 = ['a']*300
64- self.assertRaises(ValueError, self.module.StaticTuple, *args_300)
65+ self.assertRaises(TypeError, self.module.StaticTuple, *args_300)
66 # not a string
67 self.assertRaises(TypeError, self.module.StaticTuple, object())
68