| msg147480 - (view) |
Author: Florent Xicluna (flox) *  |
Date: 2011-11-12 12:43 |
| Sometimes we want to check the exact type of an object. The method assertIsInstance could be misleading in such case. The current workaround is: assertIs(type(obj), some_class) However we can add an argument to the method to keep the benefit of having a nice failure message. Examples: assertIsInstance(stdobj, dict, exact_type=True) assertIsInstance(myobj, dict, exact_type=MyDict) |
|
|
| msg147482 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2011-11-12 13:40 |
| I'm not sure this is common enough to justify a new arg. The status quo has the advantage that is quite close with what we would use in an 'if' statement (i.e. either "if isinstance(obj, some_class):" or "if type(obj) is some_class:"). |
|
|
| msg147494 - (view) |
Author: Florent Xicluna (flox) * |
Date: 2011-11-12 14:52 |
| At least in the standard library test suite, it should be useful. When the test is stricter, it catches errors earlier. I remember when assertIsInstance was made available (issue #7031), we started to rewrite some expressions self.assertEqual(type(result), str) with self.assertIsInstance(result, str) Actually, it means we relaxed the test. Today, I don't want to use assertIsInstance anymore because I want to check the exact type() of the result. The attached files list the usage of both in Lib/test/*py: - 325 assertIsInstance - 234 assert. . .type(. . .) IMHO, some assertIsInstance can be stricter. Most of the other type() tests can be replaced with this method. |
|
|
| msg147495 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-11-12 14:55 |
| I find that assertIs(type(x), someclass) is very clear, whereas an assertIsInstance that would not behave like isinstance depending on one argument would be non-obvious to me. |
|
|
| msg147501 - (view) |
Author: Florent Xicluna (flox) * |
Date: 2011-11-12 15:25 |
| I would say that this one is clear too: aelf.assertTrue(isinstance(obj, cls)) except that the failure message is not very friendly: AssertionError: False is not true If we keep assertIsInstance, more people will continue to misuse it just because the method exist, when they really want to check (type(obj) is cls). An option could be to add a snippet to the documentation of `assertIsInstance` stating that the right way to check exact type is `assertIs(type(obj), cls)`. |
|
|
| msg147504 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-11-12 15:32 |
| >I would say that this one is clear too: > aelf.assertTrue(isinstance(obj, cls)) > except that the failure message is not very friendly: > AssertionError: False is not true Yeah, you have to give something as third argument to ease debugging. > If we keep assertIsInstance, more people will continue to misuse it just because the > method exist, when they really want to check (type(obj) is cls). If they make that mistake, it is because they don’t understand what isinstance does. > An option could be to add a snippet to the documentation of `assertIsInstance` stating > that the right way to check exact type is `assertIs(type(obj), cls)`. My point was that maybe they think they really want to check the type, but with Python you don’t have to care that much most of the time. +1 on a doc addition (I can even volunteer a patch) -0.5 on the proposed new argument |
|
|
| msg147512 - (view) |
Author: Michael Foord (michael.foord) * |
Date: 2011-11-12 17:49 |
| I think your proposed workaround is good enough and no extra effort to type than the suggested change to assertIsInstance. -1 on a new method I think the behaviour of isinstance is clear enough that people who misunderstand what assertIsInstance is doing have a problem with basic Python - and will continue to make the mistake whatever we do to assertIsInstance. |
|
|
| msg147513 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2011-11-12 17:53 |
| I don't think there's a point in adding such an extra argument. Why don't you just write self.assertIs(type(myobj), sometype) ? How is the error message not good enough? |
|
|
| msg147514 - (view) |
Author: Eric Snow (eric.snow) * |
Date: 2011-11-12 18:05 |
| @ > Why don't you just write > self.assertIs(type(myobj), sometype) +1 |
|
|
| msg147523 - (view) |
Author: Florent Xicluna (flox) * |
Date: 2011-11-12 19:12 |
| > +1 on a doc addition (I can even volunteer a patch) I agree we can highlight the difference between assertIs(type(obj), cls) and assertIsInstance(obj, cls) in the documentation. Let's forget this patch and keep it simple. |
|
|
| msg147898 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2011-11-18 17:01 |
| New changeset fd9d7a8e45bc by Ezio Melotti in branch '2.7': #13387: add note about checking the exact type in assertIsInstance doc. http://hg.python.org/cpython/rev/fd9d7a8e45bc New changeset 583aff635ce1 by Ezio Melotti in branch '3.2': #13387: add note about checking the exact type in assertIsInstance doc. http://hg.python.org/cpython/rev/583aff635ce1 New changeset 196d485ed26d by Ezio Melotti in branch 'default': #13387: merge with 3.2. http://hg.python.org/cpython/rev/196d485ed26d |
|
|
| msg147899 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2011-11-18 17:01 |
| Fixed. |
|
|
| msg147903 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-11-18 17:06 |
| + To check for a specific type (without including superclasses) use + :func:`assertIs(type(obj), cls) `. Don’t you mean “without accepting subclasses”, not superclasses? |
|
|
| msg147906 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2011-11-18 17:15 |
| > + To check for a specific type (without including superclasses) use > + :func:`assertIs(type(obj), cls) `. > > Don’t you mean “without accepting subclasses”, not superclasses? I mean: >>> class MyInt(int): pass # my specific type ... >>> isinstance(MyInt(5), int) # int superclass included True >>> type(MyInt(5)) is int # int superclass not included False >>> type(MyInt(5)) is MyInt # check for specific type True Do you think I should rephrase it (or maybe just remove the (...))? |
|
|
| msg147991 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-11-20 14:28 |
| No, I’m not talking about a rephrasing, but on a full change of meaning. I don’t understand your use of “superclasses” at all; isinstance(x, T) checks if x is an instance of T or any subclass, am I wrong? |
|
|
| msg147999 - (view) |
Author: Ezio Melotti (ezio.melotti) * |
Date: 2011-11-20 16:09 |
| Is "To check for the exact type, use :func:`assertIs(type(obj), cls) `." better? I think the problem this solves is clear enough even without mentioning sub/superclasses. |
|
|
| msg148040 - (view) |
Author: Éric Araujo (eric.araujo) * |
Date: 2011-11-21 13:46 |
| Your latest proposal is better. I would prefer mentioning subclasses, but don’t feel strongly about it. One markup nit: I’d use ``code`` instead of (ab)using :func:; the doc for assertIs is just a few paragraphs above, it won’t be hard to find. |
|
|
| msg149816 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2011-12-19 05:07 |
| New changeset 88aacd3541ae by Ezio Melotti in branch '2.7': #13387: rephrase unclear sentence. http://hg.python.org/cpython/rev/88aacd3541ae New changeset eccb4795767b by Ezio Melotti in branch '3.2': #13387: rephrase unclear sentence. http://hg.python.org/cpython/rev/eccb4795767b New changeset 064854cef999 by Ezio Melotti in branch 'default': #13387: merge with 3.2. http://hg.python.org/cpython/rev/064854cef999 |
|
|