Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.9] bpo-37116: Use PEP 570 syntax for positional-only parameters. #12620

Merged
merged 11 commits into from Jun 5, 2019

Conversation

@serhiy-storchaka
Copy link
Member

commented Mar 29, 2019

Changes that can/should be made after accepting and implementing PEP 570.

https://bugs.python.org/issue37116

@gvanrossum

This comment has been minimized.

Copy link
Member

commented Mar 29, 2019

Even if the PEP were to be accepted and implemented I would be hesitant to accept this PR. It seems better to only fix a small number of places where positional-only arguments are clearly what is needed.

However I like that this PEP makes it clear that there are plenty of use cases for something to indicate positional-only arguments.

@vstinner
Copy link
Member

left a comment

Thanks. I didn't know that so many functions already implemented positonal-only arguments in pure Python (without PEP 570 syntax)! Well, to be honest, I didn't know that it was possible to implement it nor that any function was already implemented that.

"def update(*args, **kwds):" looks nice until I see the new code: "def update(self, other=(), /, **kwds):". Oh. self is missing in the current code! That's very surprising :-)

Show resolved Hide resolved Lib/collections/__init__.py Outdated
Show resolved Hide resolved Lib/unittest/mock.py Outdated
@terryjreedy
Copy link
Member

left a comment

I examined the necessity of the proposed '/' insertions in different cases divided first into whether the insertion was before **kwds or *args and secondly whether an original *args was expanded. For insertion before *args, finer distinctions seem relevant. When *args was expanded, I considered reasons it was used instead of explicit naming. In the following, ''self', 'args, and 'kwds' encompass equivalent words (such as 'cls' for 'self'). 'Other' excludes 'self' or 'cls'. Counts might be off, but are close. I hope there are no logic errors. and few typos.

Insertions before **kwds(20).

A(10): no *args. self, [other(s)],**kwds ==> self, [other(s)],/,**kwds.
Inserting '/' is not necessary. Passing self and other required ars by name is no problem as long as a call follows the standard rule that if an arg is passed by name, following args must be also. Passing self is only possible if the method is called on the class, which is almost never.

B(10): expansion. *args,**kwds ==> self, other,/**kwds.
*args is obviously inferior to the explicit expansion and delays detection of buggy calls, but it forces self and other to be called positionally. If this was intentional, then '/' should be added after expansion to maintain that aspect of the signature. To me, the result beats the original.

Insertions before *args(47)
The issue here is that named parameters before *args must be passed positionally unless nothing is passed to go into args. If any named parameters are passed by name and any objects are passed that belong in args, some will be bound to the previous parameters and a double-binding error raised. I think 'X must be passed by position' is clearer than 'X has two bindings' (where did the second come from?). I also think 'X must always be passed by position' is clearer than 'X must be passed by position except when nothing is passed for args'.

No expansion (37)
C(14): self,*args ==> self,/,*args.
Need we add '/' to catch the obscure possibility of passing self by name when the method is called on the class and additional args are passed. A double binding error would be raised anyway.
D(17): self,other(s),*args ==> self,others(s),/,*args.
E(6): other,*args ==> other,/,*args.
These are more likely for people to get wrong.

Expansion(10)
F(4): *args ==> self,/,*args. Treat same as C.
G(3) *args ==> self,other,/,*args. Treat same as D.
H(2) self,*args ==> self,other,/,*args. Treat same as D.

@serhiy-storchaka

This comment has been minimized.

Copy link
Member Author

commented Mar 31, 2019

All changes in this PR can be classified as:

  • Functions which already uses the *args hack. The benefit of the change is obvious -- significant simplification of the code, better signature. No breaking.
  • Functions which uses the naming hack (e.g. _mock_self instead of self). The change will make the code clearer and more robust. While the probability of the conflict is lower than when use just self, the change completely eliminates it. No breaking.
  • Functions which have only self or cls before *args and **kwargs. Since it is unlikely that you will pass self or cls as a keyword argument to an unbound method, and it was not guarantied (it does not work for functions implemented in C), the possibility of breaking can be ignored.
  • Functions which should not be called directly, and in all places where they are called, required arguments are passed as positional (e.g. __new__ or __init_subclass__). No breaking.
  • Functions which has other arguments before *args and **kwargs. The user can pass that arguments by name (e.g. partialmethod(func=foo, arg=bar)), although this is unlikely and never occurred in the stdlib. This is the only case where breaking is possible. We need to deprecate passing these arguments by name before making them positional-only. #12637 does this.
@@ -272,7 +272,7 @@ class _Final:

__slots__ = ('__weakref__',)

def __init_subclass__(self, *args, **kwds):
def __init_subclass__(self, /, *args, **kwds):

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Apr 2, 2019

Contributor

It was already mentioned above that it seems better to only fix a small number of places where positional-only arguments are clearly what is needed.

In other cases it looks a bit cryptic. But IIUC it is not clear that we will go this way, maybe we will decide to use the per-argument syntax with underscores.

@serhiy-storchaka serhiy-storchaka changed the title [WIP] Use PEP 570 syntax for positional-only parameters. [3.9] Use PEP 570 syntax for positional-only parameters. May 31, 2019

@serhiy-storchaka serhiy-storchaka changed the title [3.9] Use PEP 570 syntax for positional-only parameters. [3.9] bpo-37116: Use PEP 570 syntax for positional-only parameters. May 31, 2019

serhiy-storchaka added some commits Jun 1, 2019

@pablogsal

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

I think dataclasses.replace can be added to this PR.

Also all these from logging:

Lib/logging/__init__.py:def critical(msg, *args, **kwargs)
Lib/logging/__init__.py:def error(msg, *args, **kwargs)
Lib/logging/__init__.py:def exception(msg, *args, exc_info=True, **kwargs)
Lib/logging/__init__.py:def warning(msg, *args, **kwargs)
Lib/logging/__init__.py:def warn(msg, *args, **kwargs)
Lib/logging/__init__.py:def info(msg, *args, **kwargs)
Lib/logging/__init__.py:def debug(msg, *args, **kwargs)
Lib/logging/__init__.py:def log(level, msg, *args, **kwargs)
@serhiy-storchaka

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Functions in logging is not the case. They accept a limited set of keywords, and there are no conflicts with msg and level.

As for dataclasses.replace(), this is separate issue. It looks like a bug to me, and I think it should be fixed in older Python versions. PEP 570 syntax can be used since 3.8, but 3.7 needs a *args hack.

@serhiy-storchaka

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

@gvanrossum This PR was significantly changed since the initial revision. The part of changes which do not change the existing behavior but simplifies the code was committed in 3.8. This PR contains only changes for functions in which the deprecation warnings were added in 3.8. Please take a look again on it.

@gvanrossum
Copy link
Member

left a comment

Thanks for all these!

@serhiy-storchaka serhiy-storchaka merged commit 142566c into python:master Jun 5, 2019

5 checks passed

Azure Pipelines PR #20190605.15 succeeded
Details
bedevere/issue-number Issue number 37116 found
Details
bedevere/news "skip news" label found
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@serhiy-storchaka serhiy-storchaka deleted the serhiy-storchaka:use-pep-570 branch Jun 5, 2019

vsajip added a commit to vsajip/cpython that referenced this pull request Jun 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.