[proxy] web.archive.org← back | site home | direct (HTTPS) ↗ | proxy home | ◑ dark◐ light
/ cpython Public
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

bpo-36933: Remove sys.set_coroutine_wrapper (marked for removal in 3.8) #13577

Merged
merged 9 commits into from May 28, 2019

Conversation

Copy link
Contributor

Carreau commented May 25, 2019

It has been documented as deprecated and to be removed in 3.8;

From a comment on another thread – which I can't find ; leave get_coro_wrapper() for now, but always return None.

https://bugs.python.org/issue36933

Carreau changed the title bpo-3693: Remove sys.set_coro_wrapper (marked for removal in 3.8) bpo-36933: Remove sys.set_coro_wrapper (marked for removal in 3.8) May 25, 2019
Copy link
Contributor

asvetlov left a comment

Thanks for picking it up.
@1st1 wanted to do it but looks like he has no time now.

The PR is pretty good (but please fix my very minor note).

if (wrapper == NULL) {
wrapper = Py_None;
}
PyObject *wrapper = Py_None;
Copy link
Contributor

asvetlov May 26, 2019

Choose a reason for hiding this comment

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

Please use Py_RETURN_NONE here

Copy link

bedevere-bot commented May 26, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

asvetlov self-assigned this May 26, 2019
Copy link
Contributor

ZackerySpytz commented May 26, 2019

Please fix the title and commit messages of this PR. There is no such thing as "sys.set_coro_wrapper".

Carreau changed the title bpo-36933: Remove sys.set_coro_wrapper (marked for removal in 3.8) bpo-36933: Remove sys.set_coroutine_wrapper (marked for removal in 3.8) May 26, 2019
Copy link
Contributor Author

Carreau commented May 26, 2019

I have made the requested changes; please review again

I believe I've also fixed the docs building issues in CI.

Copy link

bedevere-bot commented May 26, 2019

Thanks for making the requested changes!

@asvetlov: please review the changes made to this pull request.

@@ -883,6 +883,10 @@ The following features and APIs have been removed from Python 3.8:
:func:`fileinput.FileInput` which was ignored and deprecated since Python 3.6
has been removed. :issue:`36952` (Contributed by Matthias Bussonnier)

* The function :func:`sys.set_coroutine_wrapper` deprecated in Python 3.7 has
been removed; :func:`sys.get_coroutine_wrapper` now always return ``None``.
Copy link
Member

1st1 May 27, 2019

Choose a reason for hiding this comment

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

I don't quite get it -- what's the point of keeping sys.get_coroutine_wrapper? Both should be removed.

Copy link
Contributor Author

Carreau May 27, 2019

Choose a reason for hiding this comment

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

I don't quite get it -- what's the point of keeping sys.get_coroutine_wrapper? Both should be removed.

I remember reading to remove sys.set_coroutine_wrapper now and get_coro_wrapper later... but my memory might just be failing me. I'm happy to remove get_coro_wrapper as well.

Copy link
Contributor Author

Carreau May 27, 2019

Choose a reason for hiding this comment

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

Done, and rebased (Conflict, need to regen clinic).

Copy link

bedevere-bot commented May 27, 2019

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Copy link
Contributor Author

Carreau commented May 27, 2019

I have made the requested changes; please review again

Copy link

bedevere-bot commented May 27, 2019

Thanks for making the requested changes!

@asvetlov, @1st1: please review the changes made to this pull request.

asvetlov requested a review from 1st1 May 27, 2019
asvetlov added the 🤖 automerge PR will be merged once it's been approved and all CI passed label May 28, 2019
miss-islington merged commit 3880f26 into python:master May 28, 2019
Copy link
Contributor

asvetlov commented May 28, 2019

Thanks!

Copy link
Member

1st1 commented May 28, 2019

@Carreau @asvetlov The merged commit has incorrect what's new entry. Please fix.

Copy link
Contributor

asvetlov commented May 28, 2019

Sorry for that

Copy link
Contributor

asvetlov commented May 28, 2019

@Carreau would you make a PR for post-fix?

Copy link
Contributor Author

Carreau commented May 28, 2019

@Carreau would you make a PR for post-fix?

Yes; appologies.

Carreau deleted the remove-coro-wrapper branch May 28, 2019
Copy link
Contributor Author

Carreau commented May 28, 2019

Yes; appologies.

See #13627

Copy link
Contributor

asvetlov commented May 28, 2019

No problem. Thank to @1st1 for catching this.

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
…8) (pythonGH-13577)

It has been documented as deprecated and to be removed in 3.8; 

From a comment on another thread – which I can't find ; leave get_coro_wrapper() for now, but always return `None`.


https://bugs.python.org/issue36933
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 automerge PR will be merged once it's been approved and all CI passed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants