-
-
Notifications
You must be signed in to change notification settings - Fork 30.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bpo-23867: Argument Clinic: inline parsing code for a single positional parameter. #9689
bpo-23867: Argument Clinic: inline parsing code for a single positional parameter. #9689
Conversation
e7d7228
to
2fe8d1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great work, I think adding parse_arg
to argument clinic converters is
a solid API.
It seems a lot of these converters (e.g single positional subclass_of
) are
currently not used, which means a lot of the generated code remains untested.
At the very least I think we should add as many indirect tests for the newly added
code as possible. Here's a non-exhaustive check list:
bool_converter
- from integer (tested by
curses.use_env
) - from truthy object (could be tested by
_winapi.Overlapped.GetOverlappedResult
but no tests exist for this)
char_converter
- from bytes or bytearray (could be tested by
msvcrt.putch
but no tests exist for this)
unsigned_char_converter
- from integer (tested by
curses.halfdelay
) - from unsigned long mask (unused)
short_converter
- from integer (tested by
curses.color_content
)
int_converter
- from integer (tested by
test_builtin chr()
) - from single unicode character (tested by
unicodedata.bidirectional
)
unsigned_int_converter
- from integer (unused)
long_converter
- from integer (tested by
curses.attroff
)
long_long_converter
- from integer (unused)
unsigned_long_long_converter
- from integer (unused)
Py_ssize_t_converter
- from integer (tested by
os.urandom()
)
size_t_converter
- from integer (unused)
float_converter
- from float (unused)
double_converter
- from float (tested by
math.degrees
)
Py_complex_converter
- from complex number (tested by
cmathmodule.acos
)
str_converter
- from string (tested by
codecsmodule.lookup
)
unicode_converter
- from string (used in a lot of places, didn't write test)
Py_buffer_converter
- from pybuf_simple (already tested in
marshal.loads
) - from string (already tested in
array.fromstring
) - from pybuf_writable (tested by
io.readinto1
)
For the unused ones/hard to test cases, I personally think that it would be wise to
split this change into two commits, each fully tested. One where all the single positional
ones have at least one test case, and then when argument clinic can handle generating
inline parsing code for multiple arguments/kwargs/defaults, then add the rest of the
converters with accompanying test cases.
Here are the test cases I came up with:
Test Cases
diff --git a/Lib/test/test_builtin.py b/Lib/test/test_builtin.py
index dafcf004c0..0e40829395 100644
--- a/Lib/test/test_builtin.py
+++ b/Lib/test/test_builtin.py
@@ -306,6 +306,9 @@ class BuiltinTest(unittest.TestCase):
self.assertRaises(ValueError, chr, 0x00110000)
self.assertRaises((OverflowError, ValueError), chr, 2**32)
+ with self.assertRaisesRegex(TypeError, 'integer argument expected, got float'):
+ chr(1.0)
+
def test_cmp(self):
self.assertTrue(not hasattr(builtins, "cmp"))
diff --git a/Lib/test/test_codecs.py b/Lib/test/test_codecs.py
index 00b5d317c4..529ebaf9dd 100644
--- a/Lib/test/test_codecs.py
+++ b/Lib/test/test_codecs.py
@@ -1758,6 +1758,10 @@ class CodecsModuleTest(unittest.TestCase):
self.assertRaises(TypeError, codecs.lookup)
self.assertRaises(LookupError, codecs.lookup, "__spam__")
self.assertRaises(LookupError, codecs.lookup, " ")
+ with self.assertRaisesRegex(TypeError, 'argument must be str, not list'):
+ codecs.lookup([])
+ with self.assertRaisesRegex(ValueError, 'embedded null character'):
+ codecs.lookup('abcd\0ef')
def test_getencoder(self):
self.assertRaises(TypeError, codecs.getencoder)
diff --git a/Lib/test/test_curses.py b/Lib/test/test_curses.py
index 3b442fe6a4..ae0ec49703 100644
--- a/Lib/test/test_curses.py
+++ b/Lib/test/test_curses.py
@@ -109,6 +109,9 @@ class TestCurses(unittest.TestCase):
stdscr.addnstr(4,4, '1234', 3)
stdscr.addnstr(5,5, '1234', 3, curses.A_BOLD)
+ with self.assertRaisesRegex(TypeError, 'integer argument expected, got float'):
+ stdscr.attron(1.0)
+
stdscr.attron(curses.A_BOLD)
stdscr.attroff(curses.A_BOLD)
stdscr.attrset(curses.A_BOLD)
@@ -250,6 +253,14 @@ class TestCurses(unittest.TestCase):
f.seek(0)
curses.getwin(f)
+ # Make sure type errors in halfdelay are caught
+ with self.assertRaisesRegex(TypeError, 'integer argument expected, got float'):
+ curses.halfdelay(1.0)
+ with self.assertRaisesRegex(OverflowError, 'unsigned byte integer is less than minimum'):
+ curses.halfdelay(-1)
+ with self.assertRaisesRegex(OverflowError, 'unsigned byte integer is greater than maximum'):
+ curses.halfdelay(5000)
+
curses.halfdelay(1)
curses.intrflush(1)
curses.meta(1)
@@ -272,6 +283,11 @@ class TestCurses(unittest.TestCase):
curses.unctrl('a')
curses.ungetch('a')
if hasattr(curses, 'use_env'):
+ # Make sure type errors in use_env are caught
+ with self.assertRaisesRegex(TypeError, 'integer argument expected, got float'):
+ curses.use_env(1.0)
+ with self.assertRaisesRegex(TypeError, r'an integer is required \(got type list\)'):
+ curses.use_env([])
curses.use_env(1)
# Functions only available on a few platforms
@@ -285,6 +301,14 @@ class TestCurses(unittest.TestCase):
curses.pair_content(curses.COLOR_PAIRS - 1)
curses.pair_number(0)
+ # Make sure type errors in color_content are caught
+ with self.assertRaisesRegex(TypeError, 'integer argument expected, got float'):
+ curses.color_content(1.0)
+ with self.assertRaisesRegex(OverflowError, 'signed short integer is less than minimum'):
+ curses.color_content(-100000)
+ with self.assertRaisesRegex(OverflowError, 'signed short integer is greater than maximum'):
+ curses.color_content(100000)
+
if hasattr(curses, 'use_default_colors'):
curses.use_default_colors()
@@ -301,6 +325,11 @@ class TestCurses(unittest.TestCase):
(availmask, oldmask) = curses.mousemask(curses.BUTTON1_PRESSED)
if availmask == 0:
self.skipTest('mouse stuff not available')
+
+ # type check arguments to mousemask
+ with self.assertRaisesRegex(TypeError, 'argument must be int, not float'):
+ curses.mousemask(1.0)
+
curses.mouseinterval(10)
# just verify these don't cause errors
curses.ungetmouse(0, 0, 0, 0, curses.BUTTON1_PRESSED)
diff --git a/Lib/test/test_io.py b/Lib/test/test_io.py
index c68b2fea85..97865fade9 100644
--- a/Lib/test/test_io.py
+++ b/Lib/test/test_io.py
@@ -1312,6 +1312,7 @@ class BufferedReaderTest(unittest.TestCase, CommonBufferedTests):
self.assertEqual(bufio.readinto1(b), 6)
self.assertEqual(b[:6], b"fghjkl")
self.assertEqual(rawio._reads, 4)
+ self.assertRaises(TypeError, bufio.readinto1, [])
def test_readinto_array(self):
buffer_size = 60
diff --git a/Lib/test/test_math.py b/Lib/test/test_math.py
index 9b2f55e1f4..0730738afc 100644
--- a/Lib/test/test_math.py
+++ b/Lib/test/test_math.py
@@ -271,6 +271,7 @@ class MathTests(unittest.TestCase):
def testAcos(self):
self.assertRaises(TypeError, math.acos)
+ self.assertRaises(TypeError, math.acos, 'a')
self.ftest('acos(-1)', math.acos(-1), math.pi)
self.ftest('acos(0)', math.acos(0), math.pi/2)
self.ftest('acos(1)', math.acos(1), 0)
@@ -475,6 +476,8 @@ class MathTests(unittest.TestCase):
def testDegrees(self):
self.assertRaises(TypeError, math.degrees)
+ with self.assertRaisesRegex(TypeError, 'must be real number, not str'):
+ math.degrees('a')
self.ftest('degrees(pi)', math.degrees(math.pi), 180.0)
self.ftest('degrees(pi/2)', math.degrees(math.pi/2), 90.0)
self.ftest('degrees(-pi/4)', math.degrees(-math.pi/4), -45.0)
diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py
index 6dbc255612..6bb81ab7dd 100644
--- a/Lib/test/test_os.py
+++ b/Lib/test/test_os.py
@@ -1332,6 +1332,15 @@ class URandomTests(unittest.TestCase):
data2 = self.get_urandom_subprocess(16)
self.assertNotEqual(data1, data2)
+ def test_uranom_typechecking(self):
+ with self.assertRaisesRegex(TypeError, 'integer argument expected, got float'):
+ os.urandom(1.0)
+ with self.assertRaisesRegex(TypeError, 'object cannot be interpreted as an integer'):
+ os.urandom([])
+ # This test should be rewritten to reliably overflow ssize_t on any platform
+ with self.assertRaisesRegex(OverflowError, 'Python int too large to convert to C ssize_t'):
+ os.urandom(2**128)
+
@unittest.skipUnless(hasattr(os, 'getrandom'), 'need os.getrandom()')
class GetRandomTests(unittest.TestCase):
diff --git a/Lib/test/test_unicodedata.py b/Lib/test/test_unicodedata.py
index 170778fa97..ee7587e0d8 100644
--- a/Lib/test/test_unicodedata.py
+++ b/Lib/test/test_unicodedata.py
@@ -158,6 +158,9 @@ class UnicodeFunctionsTest(UnicodeDatabaseTest):
self.assertRaises(TypeError, self.db.bidirectional)
self.assertRaises(TypeError, self.db.bidirectional, 'xx')
+ with self.assertRaisesRegex(TypeError, 'argument must be a unicode character, not list'):
+ self.db.bidirectional([])
+
def test_decomposition(self):
self.assertEqual(self.db.decomposition('\uFFFE'),'')
self.assertEqual(self.db.decomposition('\u00bc'), '<fraction> 0031 2044 0034')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review @ammaraskar.
Do you mind to add tests proposed by you in a separate PR?
https://bugs.python.org/issue23867