Skip to content

[3.10] bpo-44490: Add __parameters__ and __getitem__ to types.Union (GH-26980) #27207

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

Merged
merged 3 commits into from
Jul 17, 2021

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Jul 17, 2021

Co-authored-by: Ken Jin [email protected]
Co-authored-by: Guido van Rossum [email protected].
(cherry picked from commit c45fa1a)

Co-authored-by: Yurii Karabas [email protected]

https://bugs.python.org/issue44490

…ythonGH-26980)

Co-authored-by: Ken Jin <[email protected]>
Co-authored-by: Guido van Rossum <[email protected]>.
(cherry picked from commit c45fa1a)

Co-authored-by: Yurii Karabas <[email protected]>
@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Jul 17, 2021

Since you are backporting this, can I assume it means you approve of the original? One of the conditions by Pablo is that 2 core devs need to approve it for backport, and we already have Guido's approval.

@serhiy-storchaka
Copy link
Member Author

It includes changes made after merging the original PR.

There are two bugs left, but I am going to fix them soon.

@Fidget-Spinner
Copy link
Member

LGTM but I don't understand why make check-abidump is failing in the CI:

2 Removed functions:

  'function PyObject* _Py_make_parameters(PyObject*)'    {_Py_make_parameters}
  'function PyObject* _Py_subs_parameters(PyObject*, PyObject*, PyObject*, PyObject*)'    {_Py_subs_parameters}

Also let's wait for Pablo :).

@pablogsal
Copy link
Member

pablogsal commented Jul 17, 2021

LGTM but I don't understand why make check-abidump is failing in the CI:

2 Removed functions:

  'function PyObject* _Py_make_parameters(PyObject*)'    {_Py_make_parameters}
  'function PyObject* _Py_subs_parameters(PyObject*, PyObject*, PyObject*, PyObject*)'    {_Py_subs_parameters}

Also let's wait for Pablo :).

I am a bit uncomfortable backporting 100 lines of C for a new feature bit I will allow it based on:

  • The feature is for a new type so there is not s very high risk of backwards incompatibility.
  • The PR is authored by @serhiy-storchaka :)

But please, understand that we cannot backport new features in general, specially so close to the release candidate, so don't take this as the norm :)

@serhiy-storchaka I saw that you are preparing several bugfixes and cleanups for this type. Please, merge these ASAP as we are very close the the release candidate and I want them to be tested as much as possible.

Thanks!

@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Jul 17, 2021

But please, understand that we cannot backport new features in general, specially so close to the release candidate, so don't take this as the norm :)

@pablogsal I understand that the last beta is generally the cut-off for new features, and that this is a rare exception by you. Thanks for allowing it! I'll try my best to ensure typing-related stuff doesn't cause you any more release headaches (especially on a weekend) for the 3.10/3.11 cycle.

One unfortunate cause for things like these is that it seems that not many people actually test the betas until the last one, or when the release candidates arrive. So bugs/missing features get caught very late :(.

@pablogsal
Copy link
Member

One unfortunate cause for things like these is that it seems that not many people actually test the betas until the last one, or when the release candidates arrive. So bugs/missing features get caught very late :(.

They actually test it, but not so much for new features. The 90% of the tests they do is based on compatibility with existing code, and they don't stress test that much the new stuff

@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Jul 17, 2021

They actually test it, but not so much for new features. The 90% of the tests they do is based on compatibility with existing code, and they don't stress test that much the new stuff

Hmm I realised my original wording sounds kinda unfair on large packages that do test the betas. Yeah I meant just for the new features. (Sorry, no harm intended). I'm well aware third party packages like Django have been testing the betas since day 1 :).

@pablogsal
Copy link
Member

They actually test it, but not so much for new features. The 90% of the tests they do is based on compatibility with existing code, and they don't stress test that much the new stuff

Hmm I realised my original wording sounds kinda unfair on large packages that do test the betas. Yeah I meant just for the new features. (Sorry, no harm intended). I'm well aware third party packages like Django have been testing the betas since day 1 :).

No problem, I understood your intent ;)

@gvanrossum
Copy link
Member

I understand that the last beta is generally the cut-off for new features

Actually the first beta is the cut-off for new features.

@Fidget-Spinner
Copy link
Member

Actually the first beta is the cut-off for new features.

Oops right thanks, I blame timezones for my lapse in memory :P.

In any case, the windows x64 tests are unrelated test_asyncio failures. The make check-abidump failure doesn't make sense to me. It's saying we removed those functions from the stable ABI, but those aren't in 3.10 to begin with?

@pablogsal
Copy link
Member

pablogsal commented Jul 17, 2021

Actually the first beta is the cut-off for new features.

Oops right thanks, I blame timezones for my lapse in memory :P.

In any case, the windows x64 tests are unrelated test_asyncio failures. The make check-abidump failure doesn't make sense to me. It's saying we removed those functions from the stable ABI, but those aren't in 3.10 to begin with?

If that check if failing it means that this PR is affecting the ABI in any way, it doesn't distinguish the public or private one. Is comparing the old dump go the new one, so I'd claiming that the old one has these functions removed

@gvanrossum
Copy link
Member

Yeah, the Py_API is for exported names, not for purely internal functions.

@serhiy-storchaka serhiy-storchaka merged commit 2d055ce into python:3.10 Jul 17, 2021
@serhiy-storchaka serhiy-storchaka deleted the backport-c45fa1a-3.10 branch July 17, 2021 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants