bpo-38605: Revert making 'from future import annotations' the default by pablogsal · Pull Request #25490 · python/cpython (original) (raw)

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

pablogsal

@pablogsal

…ault

This reverts commit 044a104, adapting the code to changes that happened after it.

@gousaiyang

@vstinner

Could you give a little bit context to explain the revert? I guess that it's related to the many threads on python-dev about it?

@pablogsal

Is explained in the NEWs entry. I plan to explain it also on the final commit :)

@pablogsal

@vstinner

@isidentical

@pablogsal also please bump the magic number (cached pycs might cause some issues) (it wasn't done in the initial PR, but rather get picked up on #22630)

@pablogsal

@pablogsal also please bump the magic number (cached pycs might cause some issues) (it wasn't done in the initial PR, but rather get picked up on #22630)

We should be able to go back a version, I think. Cached pycs for alpha versions should not take lots of consideration

@pablogsal

We should be able to go back a version, I think. Cached pycs for alpha versions should not take lots of consideration

Crap, there has been quite a lot of changes since then. Ok, bumping it is!

@pablogsal

@pablogsal also please bump the magic number (cached pycs might cause some issues) (it wasn't done in the initial PR, but rather get picked up on #22630)

Done in ef38777a1f

@pablogsal

@vstinner

Oops sorry, I wanted to remove my comment rather posting it, but I closed the PR instead... I reopened the PR.

@gousaiyang

We can also update the doc of __future__ module and Lib/__future__.py to mention that annotations feature is postponed to be "mandatory in 3.11".

methane

@methane

This comment has been minimized.

@pablogsal

if (oparg & 0x04) {
assert(PyTuple_CheckExact(TOP()));
func->func_annotations = POP();
}

This line should be changed to assert(PyTuple_CheckExact(TOP()) || PyDict_CheckExact(TOP()));.
I don't know why this pull request passes the test...

Are you taking #23316 into consideration?

@methane

Are you taking #23316 into consideration?

Yes. And I know understand why this assrtion passes.

def add(a: int, b: int) -> int:
    return a + b
  3           0 LOAD_CONST               0 ('a')
              2 LOAD_NAME                0 (int)
              4 LOAD_CONST               1 ('b')
              6 LOAD_NAME                0 (int)
              8 LOAD_CONST               2 ('return')
             10 LOAD_NAME                0 (int)
             12 BUILD_TUPLE              6
             14 LOAD_CONST               3 (<code object add at 0x7f30fc37b050, file "a.py", line 3>)
             16 LOAD_CONST               4 ('add')
             18 MAKE_FUNCTION            4 (annotations)
             20 STORE_NAME               1 (add)

Annotation is tuple even when PEP 563 is disabled. That's nice.

@bedevere-bot

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 51a6f52234cf416f33a8f8ce625502dedd2bf0fd 🤖

If you want to schedule another build, you need to add the "🔨 test-with-buildbots" label again.

@pablogsal @methane

…05.9eeCNZ.rst

Co-authored-by: Inada Naoki songofacandy@gmail.com

@pablogsal

I still would like if someone can formally approve the PR :)

JelleZijlstra

methane

Choose a reason for hiding this comment

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

I have not reviewed tests yet. In other parts, LGTM.

vstinner

Choose a reason for hiding this comment

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

LGTM but please address first the comment in Lib/test/test_dataclasses.py (I concur that it looks a revert of a bugfix, so it reintroduces a typo).

I suggest to first revert the change, and then write a second PR to announce that annotations are going to change again (in Python 3.11 or later).

@pablogsal

I suggest to first revert the change, and then write a second PR to announce that annotations are going to change again (in Python 3.11 or later).

@vstinner What do you mean when you say "announce"? Announce where? In the What's new? A news entry? python-dev?

I suggest to first revert the change

Notice that this was certainly not a clean revert

@pablogsal

@vstinner

@vstinner What do you mean when you say "announce"? Announce where? In the What's new? A news entry? python-dev?

Add a What's New in Python 3.10 entry to explain that the behavior changed but was then revert, and that it will change again in a future Python version.

Something like this announcement of future collections incompatible changes (collections.Mapping alias):
https://docs.python.org/dev/whatsnew/3.9.html#you-should-check-for-deprecationwarning-in-your-code

This announce was correct: the aliases were removed again in Python 3.10.

python-dev?

Any additional communication would be good, but the minimum would be a note in the What's New in Python 3.10.

vstinner

Choose a reason for hiding this comment

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

LGTM.

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments

@vstinner vstinner vstinner approved these changes

@methane methane methane approved these changes

@ericvsmith ericvsmith Awaiting requested review from ericvsmith

@gvanrossum gvanrossum Awaiting requested review from gvanrossum

@ilevkivskyi ilevkivskyi Awaiting requested review from ilevkivskyi

@isidentical isidentical Awaiting requested review from isidentical

@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou

@markshannon markshannon Awaiting requested review from markshannon

@rhettinger rhettinger Awaiting requested review from rhettinger