Skip to content

bpo-37116: Use PEP 570 syntax for positional-only parameters.#13700

Merged
serhiy-storchaka merged 13 commits into
python:masterfrom
serhiy-storchaka:use-pep-570-38
Jun 1, 2019
Merged

bpo-37116: Use PEP 570 syntax for positional-only parameters.#13700
serhiy-storchaka merged 13 commits into
python:masterfrom
serhiy-storchaka:use-pep-570-38

Conversation

@serhiy-storchaka
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka commented May 31, 2019 •
edited by bedevere-bot
Loading

Based on #12620 but contains only changes that can be applied in 3.8. E.g. only getting rid of *args and name tricks and making self and cls positional-only.

https://bugs.python.org/issue37116

@serhiy-storchaka serhiy-storchaka changed the title Use PEP 570 syntax for positional-only parameters. bpo-37116: Use PEP 570 syntax for positional-only parameters. May 31, 2019
gvanrossum
gvanrossum approved these changes May 31, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

All LGTM. Very nice cleanups!

It looks like we technically have to add / whenever there's a **kwds (assuming the kwds is passed through and could contain any key)...

Comment thread Lib/string.py
import re as _re
from collections import ChainMap as _ChainMap

_sentinel_dict = {}
Copy link
Copy Markdown
Member

@gvanrossum gvanrossum May 31, 2019

Choose a reason for hiding this comment

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

Why not use object()? IIUC all the code below checks for its identity first and then substitutes another dict.

Copy link
Copy Markdown
Member Author

@serhiy-storchaka serhiy-storchaka Jun 1, 2019

Choose a reason for hiding this comment

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

Because semantically the default value for the mapping parameter is an empty dict. The check mapping is _sentinel_dict is merely an optimization (we can avoid to create a ChainMap in this case).

Comment thread Lib/weakref.py
tirkarthi
tirkarthi reviewed May 31, 2019
View reviewed changes
Comment thread Lib/unittest/case.py
@pablogsal
Copy link
Copy Markdown
Member

pablogsal commented May 31, 2019

Thank you very much @serhiy-storchaka for working on this 🎉

tirkarthi
tirkarthi reviewed May 31, 2019
View reviewed changes
Comment thread Lib/unittest/mock.py
tirkarthi
tirkarthi reviewed May 31, 2019
View reviewed changes
Comment thread Lib/unittest/mock.py
ilevkivskyi
ilevkivskyi approved these changes May 31, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

typing/abc/collections code LGTM.

@serhiy-storchaka
Copy link
Copy Markdown
Member Author

serhiy-storchaka commented Jun 1, 2019 •
edited by bedevere-bot
Loading

Yes, we can add / in most cases whenever there's a **kwds.

With bpo-36518 we should not even add it if all positional arguments are required (this covers the majority of cases).

@serhiy-storchaka
Copy link
Copy Markdown
Member Author

serhiy-storchaka commented Jun 1, 2019

@pablogsal The two last bugs related to positional-only arguments I found when worked on this issue.

tirkarthi
tirkarthi approved these changes Jun 1, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@tirkarthi tirkarthi left a comment

Choose a reason for hiding this comment

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

mock changes LGTM. There are few places where _mock_self is used in the async helpers later assigning to self following older pattern. I will try to change them in another PR so that this is good for beta 1. Thanks Serhiy.

@serhiy-storchaka serhiy-storchaka requested a review from a team as a code owner June 1, 2019 07:38
@serhiy-storchaka serhiy-storchaka merged commit 2085bd0 into python:master Jun 1, 2019
@serhiy-storchaka serhiy-storchaka deleted the use-pep-570-38 branch June 1, 2019 08:00
@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Jun 1, 2019

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Windows7 3.x has failed when building commit 2085bd0.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/58/builds/2538) and take a look at the build logs.
  4. Check if the failure is related to this commit (2085bd0) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/58/builds/2538

Click to see traceback logs 
Traceback (most recent call last):
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\test\test_venv.py", line 327, in test_multiprocessing
    out, err = check_output([envpy, '-c',
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\test\test_venv.py", line 42, in check_output
    raise subprocess.CalledProcessError(
subprocess.CalledProcessError: Command '['d:\\temp\\tmpzz1z4c8u\\Scripts\\python_d.exe', '-c', 'from multiprocessing import Pool; print(Pool(1).apply_async("Python".lower).get(3))']' returned non-zero exit status 3221225477.

----------------------------------------------------------------------

Ran 16 tests in 144.483s

FAILED (errors=1, skipped=2)


Traceback (most recent call last):
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\test\test_venv.py", line 327, in test_multiprocessing
    out, err = check_output([envpy, '-c',
  File "D:\cygwin\home\db3l\buildarea\3.x.bolen-windows7\build\lib\test\test_venv.py", line 42, in check_output
    raise subprocess.CalledProcessError(
subprocess.CalledProcessError: Command '['d:\\temp\\tmp_qt0ywa6\\Scripts\\python_d.exe', '-c', 'from multiprocessing import Pool; print(Pool(1).apply_async("Python".lower).get(3))']' returned non-zero exit status 3221225477.

----------------------------------------------------------------------

Ran 16 tests in 65.325s

FAILED (errors=1, skipped=2)

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@serhiy-storchaka @DinoV
bpo-37116: Use PEP 570 syntax for positional-only parameters. (python…
6ac94bc 
…GH-13700)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tirkarthi tirkarthi tirkarthi approved these changes

@gvanrossum gvanrossum gvanrossum approved these changes

@ilevkivskyi ilevkivskyi ilevkivskyi approved these changes

@1st1 1st1 Awaiting requested review from 1st1

@ericvsmith ericvsmith Awaiting requested review from ericvsmith

@rhettinger rhettinger Awaiting requested review from rhettinger

@terryjreedy terryjreedy Awaiting requested review from terryjreedy

+1 more reviewer

@auvipy auvipy auvipy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@serhiy-storchaka @pablogsal @bedevere-bot @gvanrossum @tirkarthi @auvipy @ilevkivskyi @the-knights-who-say-ni