Skip to content

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented May 31, 2019

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
Copy link
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)...

@@ -52,6 +52,8 @@ def capwords(s, sep=None):
import re as _re
from collections import ChainMap as _ChainMap

_sentinel_dict = {}
Copy link
Member

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
Member Author

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).

@pablogsal
Copy link
Member

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

Copy link
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
Member Author

serhiy-storchaka commented Jun 1, 2019

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
Member Author

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

Copy link
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

⚠️⚠️⚠️ 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants