Skip to content

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

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 9 commits into from
May 28, 2019

Conversation

Carreau
Copy link
Contributor

@Carreau 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 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 asvetlov left a comment

Choose a reason for hiding this comment

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

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

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

@bedevere-bot
Copy link

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 asvetlov self-assigned this May 26, 2019
@ZackerySpytz
Copy link
Contributor

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

@Carreau 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
@Carreau
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.

@bedevere-bot
Copy link

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

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

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

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

@bedevere-bot
Copy link

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.

@Carreau Carreau force-pushed the remove-coro-wrapper branch from 0920be7 to 409ac4a Compare May 27, 2019 16:45
@Carreau
Copy link
Contributor Author

Carreau commented May 27, 2019

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@asvetlov
Copy link
Contributor

Thanks!

@1st1
Copy link
Member

1st1 commented May 28, 2019

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

@asvetlov
Copy link
Contributor

Sorry for that

@asvetlov
Copy link
Contributor

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

@Carreau
Copy link
Contributor Author

Carreau commented May 28, 2019

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

Yes; appologies.

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

Carreau commented May 28, 2019

Yes; appologies.

See #13627

@asvetlov
Copy link
Contributor

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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants