bpo-33197: Update error message of invalid inspect.Parameters by corona10 · Pull Request #6636 · 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

Conversation62 Commits1 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 }})

The error message from constructing invalid inspect.
Parameters was confused due to print integer enum value (parameter kind).
It is updated to print a string value of parameter kind.

https://bugs.python.org/issue33197

For reviewers,

Please add labels needs backport to 3.7/3.6

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind can be not only _ParameterKind member here, but an integer. You need first make a _ParameterKind from it: _ParameterKind(kind).

There is the same formatting error at line 2465. The message at line 2739 can be improved too.

The message at line 2456 should contain the name of the type instead of the repr of the value.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

corona10 changed the title~~bpo-33197: Update error message of invalid inspect.Parameters~~ [WIP] bpo-33197: Update error message of invalid inspect.Parameters

Apr 30, 2018

corona10 changed the title~~[WIP] bpo-33197: Update error message of invalid inspect.Parameters~~ bpo-33197: Update error message of invalid inspect.Parameters

Apr 30, 2018

I have made the requested changes; please review again

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a regression from blindly converting everything in stdlib to use enums. Thanks for fixing it!

I don't like the 'VAR_KEYWORD cannot have default values' message, it's rather cryptic. Please add a mapping of parameter-kinds to human-readable form and use that mapping to render good-looking error messages. Thanks!

if kind in (_VAR_POSITIONAL, _VAR_KEYWORD):
msg = '{} parameters cannot have default values'.format(kind)
if self._kind in (_VAR_POSITIONAL, _VAR_KEYWORD):
msg = f'{self._kind!s} cannot have default values'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove 'parameters' word from the message?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@1st1
No reason for it, I revert it.

corona10 changed the title~~bpo-33197: Update error message of invalid inspect.Parameters~~ [WIP]bpo-33197: Update error message of invalid inspect.Parameters

May 1, 2018

corona10 changed the title~~[WIP]bpo-33197: Update error message of invalid inspect.Parameters~~ bpo-33197: Update error message of invalid inspect.Parameters

May 1, 2018

@1st1

Please take a look if you have a time 👍

@@ -1444,6 +1444,19 @@ def f6(a, b, c):
with self.assertRaisesRegex(TypeError, "'a', 'b' and 'c'"):
inspect.getcallargs(f6)

# bpo-33197
with self.assertRaisesRegex(ValueError,
'variadic keyword parameters cannot have default values'):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long line

inspect.Parameter("foo", kind=inspect.Parameter.VAR_KEYWORD,
default=42)
with self.assertRaisesRegex(ValueError,
"invalid value for 'Parameter.kind' attribute"):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long line

@1st1

Thanks you for reviewing all 79 characters linting issues.
Now there are no longer characters over 79 characters on this patch
including inspect.py, test_inspect.py and news.

I should care about this linting convention from now on.

Thank you for redundunt reviewing for same issues :)

Enjoy your weekend! thank you

self._kind = _ParameterKind(kind)
except ValueError:
msg = "invalid value for 'Parameter.kind' attribute"
raise ValueError(msg) from None

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last change here:

raise ValueError(f'value {kind!r} is not a valid Parameter.kind')

if self._kind != _POSITIONAL_OR_KEYWORD:
msg = (
'implicit arguments must be passed in as '
'{} arguments not {} arguments'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"implicit arguments must be passed as positional or keyword arguments, not {}"

1st1 approved these changes May 27, 2018

Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple nitpicks, please fix them. LGTM otherwise.

The error message from constructing invalid inspect. Parameters was confused due to print integer enum value (parameter kind). It is updated to print a string value of parameter kind.

1st1 approved these changes May 29, 2018