bpo-29951: Include function name for some error messages in PyArg_ParseTupleAndKeywords by MSeifert04 · Pull Request #916 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation20 Commits4 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Issue: 29951
I wasn't sure if this needs to be tested and if there is a dedicated test-file for these functions.
Include function name for some error messages in PyArg_ParseTupleAndKeywords bpo-29951: Include function name for some error messages in PyArg_ParseTupleAndKeywords
| @@ -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" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| @@ -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()" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
msg is a regular expression. It is matched only by accident. Either escape or remove ().
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oups. Thanks for spotting this.
If escaped it also needs to be a raw string then, right?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
| @@ -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"): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks as the function has the name "required". May be omit "for function" if function name is not specified?
@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.
| " (%d given)", |
|---|
| (fname == NULL) ? "function" : fname, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested this for uniformity with other error messages.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I didn’t notice, and it’s not really a big deal. It just seemed a style preference thing to me.
| @@ -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 |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I have two style questions.
| @@ -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 " |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“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.
| @@ -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') " |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“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.
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?
I don't know. @vadmium, what are your thoughts? The patch LGTM in current form and will LGTM with any of these modifications.
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.