gh-127610: Added validation for more than one var positional and var keyword parameters in inspect.Signature#127657
Conversation
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
skirpichev
left a comment
There was a problem hiding this comment.
And please add news. This is tiny, but user-visible change.
Lib/inspect.py
Outdated
| var_positional_count += 1 | ||
| if var_positional_count > 1: |
There was a problem hiding this comment.
It seems, *_count actually didn't count anything, isn't? I suggest use bool values instead and rename variables to something like seen_var_positional.
Lib/inspect.py
Outdated
| if kind == _VAR_POSITIONAL: | ||
| var_positional_count += 1 | ||
| if var_positional_count > 1: | ||
| msg = 'more than one var positional parameter' |
There was a problem hiding this comment.
Per glossary, probably it is better to use "var-positional" (and "var-keyword") terms.
|
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
skirpichev
left a comment
There was a problem hiding this comment.
LGTM, with a small nitpick.
Misc/NEWS.d/next/Library/2024-12-06-17-28-55.gh-issue-127610.ctv_NP.rst
Outdated
Show resolved
Hide resolved
…tv_NP.rst Co-authored-by: Sergey B Kirpichev <skirpichev@gmail.com>
serhiy-storchaka
left a comment
There was a problem hiding this comment.
You do not actually need seen_var_positional and seen_var_keyword. seen_var_positional is equal to topkind >= _VAR_POSITIONAL (before setting topkind = kind).
You can add if kind == top_kind and kind in (_VAR_POSITIONAL, _VAR_KEYWORD) before elif kind > top_kind. Use kind.description to format the error message.
| second_args = args.replace(name="second_args") | ||
| with self.assertRaisesRegex(ValueError, 'more than one var-positional parameter'): | ||
| S((args, second_args)) |
There was a problem hiding this comment.
Add also a test for the case when there are other parameters (keyword-only or var-keyword) between two var-positional parameters.
There was a problem hiding this comment.
In this case we will get error: ValueError: wrong parameter order: keyword-only parameter before variadic positional parameter. Should we catch it in the test?
My test:S((args, ko, second_args))
There was a problem hiding this comment.
Technically, there are two errors: "keyword-only parameter before variadic positional parameter" and "more than one variadic positional parameter". Which of them are preferable to report? I think the latter, therefore we should change the order of checks. And tests are needed to ensure that the correct error message is used.
There was a problem hiding this comment.
In that case we all need the variables seen_var_positional and seen_var_keyword, to check that these parameters have not occurred before even if the parameter order is wrong, right?
if kind == top_kind and kind in (_VAR_POSITIONAL, _VAR_KEYWORD) - this check is designed for the correct order of parameters
There was a problem hiding this comment.
Hmm, yes, you are right. So you need to simply move the new checks up. I still suggest to use kind.description to format the error message.
You can unify the code for var-positional and var-keyword if use a set instead of two boolean variables.
if kind in (_VAR_POSITIONAL, _VAR_KEYWORD):
if kind in seen_var_parameters:
raise ...
seen_var_parameters.add(kind)
Misc/NEWS.d/next/Library/2024-12-06-17-28-55.gh-issue-127610.ctv_NP.rst
Outdated
Show resolved
Hide resolved
…tv_NP.rst Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
serhiy-storchaka
left a comment
There was a problem hiding this comment.
LGTM. 👍
You can use f-string for formatting the error message.
picnixz
left a comment
There was a problem hiding this comment.
I'm ok with this current form and you can change to f-strings as Serhiy said if you want (but forget about my set vs tuple suggestion)
|
|
|
@serhiy-storchaka what do you think about backports to 3.13 and 3.12? FYI: buildbots failures are unrelated, it seems that there is some kind of network issue. |
|
…d var-keyword parameters in inspect.Signature (pythonGH-127657)
Added flags for var positional and var keyword parameters.
Raise ValueError if arguments are more than one.
Added corresponding tests