bpo-30149: Fix partialmethod without explicit self parameter. by corona10 · Pull Request #1308 · python/cpython

corona10 · 2017-04-26T17:52:57Z

bpo-30149: Fix partialmethod without explicit self parameter.

mention-bot · 2017-04-26T17:53:01Z

@corona10, thanks for your PR! By analyzing the history of the files in this pull request, we identified @1st1, @larryhastings and @zestyping to be potential reviewers.

corona10 · 2017-04-26T18:20:29Z

For reviewers,
test_imaplib is commonly failed includes other PR now.
This issue is not related to this PR.

serhiy-storchaka

Thank you @corona10. I didn't expect a fix so fast.

I'm not an expert of the inspect module and left only style notes and questions.

serhiy-storchaka · 2017-04-26T18:13:31Z

Lib/test/test_inspect.py

Follow the style of other tests and move the expected result on the next line.

serhiy-storchaka · 2017-04-26T18:18:12Z

Lib/test/test_inspect.py

I think it would look clearer if use a local function instead of a lambda.

Are there other tests for partialmethod? If not, it would be nice to add tests for more common partialmethod.

Does this work if there are keyword-only parameters after *args? Does this work if a partialmethod is decorated with classmethod or staticmethod?

I updated it with adding the case you mentioned!

1st1 · 2017-04-26T22:49:57Z

Lib/inspect.py

I'd actually prefer you checking if first_wrapped_param is VAR_POSITIONAL:

sig_params = tuple(sig.parameters.values()) if first_wrapped_param.kind is Parameter.VAR_POSITIONAL: new_params = sig_params else: assert first_wrapped_param is not sig_params[0] new_params = (first_wrapped_param,) + sig_params

@1st1
I should read pep-0362 before fixing this. Thanks!

corona10 · 2017-04-27T06:51:09Z

@1st1
Thank you for reviewing my PR. You approach is much better than I did.
I updated it and soon going to update unit tests which @serhiy-storchaka guided.

corona10 · 2017-04-27T08:29:28Z

@1st1 @serhiy-storchaka
Updated it, Can you take a look?

1st1 · 2017-04-27T19:33:28Z

Lib/inspect.py

Just realized that it's a bit inefficient to construct a new signature object with an unmodified copy of its parameters.

Put a return sig here, and move sig_params = tuple(sig.parameters.values()) to the else block...

1st1 · 2017-04-27T19:33:46Z

Lib/inspect.py

... and return sig.replace(parameters=new_params) here ...

1st1 · 2017-04-27T19:34:13Z

Lib/inspect.py

... and this return can be then removed.

corona10 · 2017-04-28T01:42:04Z

@1st1
Updated! Please take a look

corona10 · 2017-04-28T10:35:02Z

I will improve this diff codecov into 100% today.

corona10 · 2017-04-28T14:24:38Z

Lib/test/test_inspect.py

@serhiy-storchaka @1st1
Codecov complains that they want to add unit test codes for these functions(e.g test_args_only, test_args_kwargs_only ) which are just only for test.
I think that add unit tests code for test code is not reasonable.
What do you think?

Ignore this.

You can put # NOQA like this:

def test_args_only(*args): # NOQA pass

@1st1
Thank you for the tip!
I will update it right now!

1st1 · 2017-04-28T15:35:37Z

Lib/inspect.py

One last thing: we need a short comment explaining this branch:

# The first argument of the wrapped callable is `*args`, # as in `partialmethod(lambda *args)`. return sig

That makes sense!

1st1 · 2017-04-28T15:38:19Z

Lib/test/test_inspect.py

Two spaces between : and # please (PEP8).

corona10 · 2017-04-28T15:57:08Z

@1st1
Updated and all passed.
PTAL

1st1 · 2017-05-01T21:14:56Z

@corona10 Could you please add a NEWS entry?

corona10 · 2017-05-01T23:37:54Z

@1st1 Thanks, I updated a NEWS entry.

serhiy-storchaka · 2017-05-02T18:29:39Z

Misc/NEWS

This is very unusual indentation. Make the entry conforming the style of other entries.

And please use two spaces after a sentence ending period.

Change this to:

- bpo-30149: inspect.signature() now supports callables with variable-argument parameters wrapped with partialmethod. Patch by Dong-hee Na.

@serhiy-storchaka Sorry for the mistake. I didn't notice that. I updated it!

@1st1 Thanks!

corona10 · 2017-05-05T13:35:40Z

@1st1 @serhiy-storchaka Can you take a look if you are okay?

1st1 · 2017-05-05T14:15:30Z

It's ready, I'll merge it today.

serhiy-storchaka · 2017-05-05T17:44:38Z

If this is good to @1st1, it is good to me.

corona10 · 2017-05-15T12:58:17Z

@1st1 If you want to merge it. Please ping me. I will rebase it quickly.

…1308)

…1662)

…1663)

the-knights-who-say-ni added the CLA signed label Apr 26, 2017

corona10 force-pushed the bpo-30149 branch 2 times, most recently from be30903 to 4cc46f8 Compare April 26, 2017 18:02

serhiy-storchaka requested review from 1st1 and serhiy-storchaka April 26, 2017 18:10

serhiy-storchaka reviewed Apr 26, 2017

View reviewed changes

1st1 added the needs backport to 3.6 label Apr 26, 2017

1st1 requested changes Apr 26, 2017

View reviewed changes

corona10 force-pushed the bpo-30149 branch from 4cc46f8 to 38644dd Compare April 27, 2017 00:58

corona10 changed the title ~~bpo-30149: Fix partialmethod without explicit self parameter.~~ [WIP] bpo-30149: Fix partialmethod without explicit self parameter. Apr 27, 2017

corona10 force-pushed the bpo-30149 branch from 38644dd to abe66cd Compare April 27, 2017 07:56

corona10 changed the title ~~[WIP] bpo-30149: Fix partialmethod without explicit self parameter.~~ bpo-30149: Fix partialmethod without explicit self parameter. Apr 27, 2017

1st1 reviewed Apr 27, 2017

View reviewed changes

Lib/inspect.py Outdated

Copy link

Member

1st1 Apr 27, 2017 •

edited

Loading

Choose a reason for hiding this comment

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

... and return sig.replace(parameters=new_params) here ...

1st1 reviewed Apr 27, 2017

View reviewed changes

Lib/inspect.py Outdated

Copy link

Member

1st1 Apr 27, 2017

Choose a reason for hiding this comment

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

... and this return can be then removed.

corona10 force-pushed the bpo-30149 branch from abe66cd to c1e8cf6 Compare April 28, 2017 01:41

corona10 force-pushed the bpo-30149 branch 2 times, most recently from 8448b72 to 6de6e6c Compare April 28, 2017 12:43

corona10 commented Apr 28, 2017

View reviewed changes

1st1 reviewed Apr 28, 2017

View reviewed changes

corona10 force-pushed the bpo-30149 branch from 6de6e6c to debc6f5 Compare April 28, 2017 15:36

1st1 reviewed Apr 28, 2017

View reviewed changes

Lib/test/test_inspect.py Outdated

Copy link

Member

1st1 Apr 28, 2017

Choose a reason for hiding this comment

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

Two spaces between : and # please (PEP8).

corona10 force-pushed the bpo-30149 branch from debc6f5 to c98a588 Compare April 28, 2017 15:41

1st1 added the needs backport to 3.5 label May 1, 2017

1st1 approved these changes May 1, 2017

View reviewed changes

corona10 force-pushed the bpo-30149 branch 2 times, most recently from 2cc2379 to fe3da51 Compare May 1, 2017 23:37

serhiy-storchaka reviewed May 2, 2017

View reviewed changes

corona10 force-pushed the bpo-30149 branch 3 times, most recently from a91813d to d28885b Compare May 3, 2017 13:56

bpo-30149: Fix partialmethod without explicit self parameter

ec4c9b4

corona10 force-pushed the bpo-30149 branch from d28885b to ec4c9b4 Compare May 5, 2017 13:34

serhiy-storchaka assigned 1st1 May 5, 2017

Merge branch 'master' into bpo-30149

dd66c1a

1st1 merged commit 378d706 into python:master May 17, 2017

corona10 added a commit to corona10/cpython that referenced this pull request May 19, 2017

bpo-30149: Fix partialmethod without explicit self parameter (python#…

97c4ebc

…1308)

corona10 added a commit to corona10/cpython that referenced this pull request May 19, 2017

bpo-30149: Fix partialmethod without explicit self parameter (python#…

4e16795

…1308)

This was referenced May 19, 2017

[3.6] bpo-30149: Fix partialmethod without explicit self parameter (#1308) #1662

Merged

[3.5] bpo-30149: Fix partialmethod without explicit self parameter (#1308) #1663

Merged

serhiy-storchaka pushed a commit that referenced this pull request Jun 15, 2017

bpo-30149: Fix partialmethod without explicit self parameter (#1308) (#…

e45ea37

…1662)

serhiy-storchaka pushed a commit that referenced this pull request Jun 15, 2017

bpo-30149: Fix partialmethod without explicit self parameter (#1308) (#…

fb0825c

…1663)

Mariatta removed needs backport to 3.5 labels Jun 17, 2017

corona10 deleted the bpo-30149 branch October 10, 2017 00:47

Uh oh!

Conversation

corona10 commented Apr 26, 2017

Uh oh!

mention-bot commented Apr 26, 2017

Uh oh!

corona10 commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

corona10 commented Apr 27, 2017

Uh oh!

corona10 commented Apr 27, 2017

Uh oh!

1st1 Apr 27, 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!

1st1 Apr 27, 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!

corona10 commented Apr 28, 2017

Uh oh!

corona10 commented Apr 28, 2017

Uh oh!

corona10 Apr 28, 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!

1st1 Apr 28, 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!

corona10 commented Apr 28, 2017

Uh oh!

1st1 commented May 1, 2017

Uh oh!

corona10 commented May 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

corona10 commented May 5, 2017

Uh oh!

1st1 commented May 5, 2017

Uh oh!

serhiy-storchaka commented May 5, 2017

Uh oh!

corona10 commented May 15, 2017

corona10 commented Apr 26, 2017 •

edited

Loading

1st1 Apr 27, 2017 •

edited

Loading

1st1 Apr 27, 2017 •

edited

Loading

corona10 Apr 28, 2017 •

edited

Loading

1st1 Apr 28, 2017 •

edited

Loading

corona10 commented May 1, 2017 •

edited

Loading