bpo-29951: Include function name for some error messages in PyArg_ParseTupleAndKeywords #916

MSeifert04 · 2017-03-31T00:04:20Z

I wasn't sure if this needs to be tested and if there is a dedicated test-file for these functions.

mention-bot · 2017-03-31T00:04:22Z

@MSeifert04, thanks for your PR! By analyzing the history of the files in this pull request, we identified @serhiy-storchaka, @exarkun and @loewis to be potential reviewers.

…seTupleAndKeywords

serhiy-storchaka · 2017-03-31T05:37:57Z

Python/getargs.c

@@ -1705,8 +1705,10 @@ vgetargskeywords(PyObject *args, PyObject *kwargs, const char *format,
            }
            if (max < nargs) {
                PyErr_Format(PyExc_TypeError,
-                             "Function takes %s %d positional arguments"
+                             "%s%s takes %s %d positional arguments"


Use %.200s instead of just %s for function name.

Also changed format specifier for function name from "%s" to "%.200s" and exception messages should start with lowercase letter

serhiy-storchaka · 2017-03-31T09:08:24Z

Lib/test/test_exceptions.py

@@ -1089,7 +1089,7 @@ def test_attributes(self):
        self.assertEqual(exc.name, 'somename')
        self.assertEqual(exc.path, 'somepath')

-        msg = "'invalid' is an invalid keyword argument for this function"
+        msg = "'invalid' is an invalid keyword argument for ImportError()"


msg is a regular expression. It is matched only by accident. Either escape or remove ().

Oups. Thanks for spotting this.

If escaped it also needs to be a raw string then, right?

Yes, if you decide to escape (). But you can just remove for ImportError(). This message can be changed in future again (for example to for ImportError constructor).

serhiy-storchaka · 2017-03-31T09:16:43Z

Lib/test/test_getargs2.py

@@ -626,16 +626,16 @@ def test_required_args(self):
            )
        # required arg missing
        with self.assertRaisesRegex(TypeError,
-            r"Required argument 'required' \(pos 1\) not found"):
+            r"required argument for function 'required' \(pos 1\) not found"):


This looks as the function has the name "required". May be omit "for function" if function name is not specified?

exarkun · 2017-03-31T10:40:38Z

@MSeifert04, thanks for your PR! By analyzing the history of the files in this pull request, we identified @serhiy-storchaka, @exarkun and @loewis to be potential reviewers.

Please remove me from the mention-bot configuration.

vadmium · 2017-03-31T11:47:05Z

Python/getargs.c

                             " (%d given)",
+                             (fname == NULL) ? "function" : fname,


Any reason why you changed from uppercase Function to lowercase “function”?

It may be worth changing an exception message if you can make it less confusing or easier to read. But changing just the letter case does not seem worth the pain of breaking the old message.

I suggested this for uniformity with other error messages.

Okay, I didn’t notice, and it’s not really a big deal. It just seemed a style preference thing to me.

serhiy-storchaka · 2017-03-31T12:00:25Z

Lib/test/test_extcall.py

@@ -221,7 +221,7 @@
    >>> f(**{1:2})
    Traceback (most recent call last):
      ...
-    TypeError: f() keywords must be strings
+    TypeError: f() keyword arguments must be strings


After you have changed this I see that the former wording was more correct. I think it would be better to change the error message in PyArg_ValidateKeywordArguments() rather than in all other code.

serhiy-storchaka

LGTM, but I have two style questions.

serhiy-storchaka · 2017-03-31T16:26:01Z

Python/getargs.c

@@ -1752,8 +1754,10 @@ vgetargskeywords(PyObject *args, PyObject *kwargs, const char *format,
                     * or the end of the format. */
                }
                else {
-                    PyErr_Format(PyExc_TypeError, "Required argument "
-                                 "'%s' (pos %d) not found",
+                    PyErr_Format(PyExc_TypeError,  "%.200s%s missing required "


Wouldn't be better to omit the word "function" if the function name is not specified? Just "missing required argument 'foo' (pos 2)". What are your thoughts? Is it correct English and does it look better?

I know that almost all other error messages used "function" or "this function" as placeholder when there's no name. But, yes, I mostly included it for consistency reasons, I have no preference either way.

(Note: I'm non native speaker so I would have to guess about correctness and/or looks)

“Missing required argument ‘foo’ (pos 2)” is correct English.

Whether including “function” may help would depend on the context. It may help when it is not clear from the exception context that a function call is involved. On the other hand, omitting the word makes the message simpler when the context is already clear.

serhiy-storchaka · 2017-03-31T16:27:46Z

Python/getargs.c

@@ -1802,8 +1808,10 @@ vgetargskeywords(PyObject *args, PyObject *kwargs, const char *format,
            if (current_arg) {
                /* arg present in tuple and in dict */
                PyErr_Format(PyExc_TypeError,
-                             "Argument given by name ('%s') "
+                             "argument for %.200s%s given by name ('%s') "


Wouldn't be better to omit the words "for function" if the function name is not specified? Just "argument given by name ('foo') and position (2)". What are your thoughts?

“For function” probably doesn’t add much value to the exception messsage in this case. But it does not seem very important to remove it either: it is a judgement call.

MSeifert04 · 2017-04-03T13:59:59Z

Is there anything that needs to be done?

I'm not sure about the remaining comments. Should I revert the "capitalization" changes and should I remove the "function" stubs or are these "optional" comments?

serhiy-storchaka · 2017-04-05T18:43:53Z

I don't know. @vadmium, what are your thoughts? The patch LGTM in current form and will LGTM with any of these modifications.

vadmium · 2017-04-09T00:29:03Z

I think these questions are just a judgement call for whoever writes the code and whoever accepts it. If it were just me, I tend to start exception messages with a capital letter so that the message stands better on its own, but I would also respect the existing style, and avoid changing exception messages unless they are significantly better. So if you think the new messages are clearer or have a more consistent style, go for it.

the-knights-who-say-ni added the CLA signed label Mar 31, 2017

MSeifert04 changed the title ~~Include function name for some error messages in PyArg_ParseTupleAndKeywords~~ bpo-29951: Include function name for some error messages in PyArg_ParseTupleAndKeywords Mar 31, 2017

bpo-29951: Include function name for some error messages in PyArg_Par…

60405e8

…seTupleAndKeywords

serhiy-storchaka reviewed Mar 31, 2017

View reviewed changes

bpo-29951: Include function name for PyArg_ParseTupleAndKeywords errors

5330e2c

Also changed format specifier for function name from "%s" to "%.200s" and exception messages should start with lowercase letter

serhiy-storchaka reviewed Mar 31, 2017

View reviewed changes

bpo-29951: Changed exception message for missing required argument

e4f4c24

vadmium reviewed Mar 31, 2017

View reviewed changes

serhiy-storchaka reviewed Mar 31, 2017

View reviewed changes

bpo-29551: undo the "keyword argument" part of the change

4da0e03

serhiy-storchaka reviewed Mar 31, 2017

View reviewed changes

brettcannon added the type-feature A feature request or enhancement label Mar 31, 2017

serhiy-storchaka approved these changes Apr 5, 2017

View reviewed changes

serhiy-storchaka requested a review from vadmium April 5, 2017 18:44

serhiy-storchaka merged commit 64c8f70 into python:master Apr 9, 2017

MSeifert04 deleted the functionname_errormsg_tupleandkw branch April 9, 2017 12:42

SylvainDe mentioned this pull request Apr 11, 2017

Different regexp on Python 3.7-dev/nightly SylvainDe/DidYouMean-Python#29

Closed

Uh oh!

bpo-29951: Include function name for some error messages in PyArg_ParseTupleAndKeywords #916

bpo-29951: Include function name for some error messages in PyArg_ParseTupleAndKeywords #916

Uh oh!

Conversation

MSeifert04 commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mention-bot commented Mar 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

exarkun commented Mar 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSeifert04 commented Apr 3, 2017

Uh oh!

serhiy-storchaka commented Apr 5, 2017

Uh oh!

vadmium commented Apr 9, 2017

Uh oh!

Uh oh!

MSeifert04 commented Mar 31, 2017 •

edited

Loading

MSeifert04 Mar 31, 2017 •

edited

Loading

MSeifert04 Mar 31, 2017 •

edited

Loading