Skip to content
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.

Sign up for GitHub

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

Jump to bottom

bpo-35283: Add a deprecated warning for the threading.Thread.isAlive #11454

Merged
merged 1 commit into from
Jan 17, 2019
Merged

bpo-35283: Add a deprecated warning for the threading.Thread.isAlive #11454

merged 1 commit into from
Jan 17, 2019

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Jan 7, 2019 •
edited by bedevere-bot
Loading

@corona10
Copy link
Member Author

corona10 commented Jan 7, 2019

@asvetlov

Hi, Can you take a look, please?

eamanu
eamanu approved these changes Jan 7, 2019
View reviewed changes
Copy link
Contributor

@eamanu eamanu left a comment

Choose a reason for hiding this comment

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

LGTM

Lib/test/test_threading.py Outdated Show resolved Hide resolved
@corona10
Copy link
Member Author

corona10 commented Jan 7, 2019

@eamanu Done thanks!

@corona10 corona10 mentioned this pull request Jan 7, 2019
Closed
@corona10 corona10 force-pushed the bpo-35283 branch 3 times, most recently from fd44c33 to b9c06ba Compare January 7, 2019 16:31
@corona10 corona10 changed the title bpo-35283: Add a _DummyThread.isAlive alias and a deprecated msg. bpo-35283: Add a deprecated warning for the threading.Thread.isAlive Jan 7, 2019
@corona10
Copy link
Member Author

@vstinner Hi, Could you review this PR, please?

vstinner
vstinner reviewed Jan 16, 2019
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Please document the deprecation at https://docs.python.org/dev/whatsnew/3.8.html#deprecated : Doc/whatsnew/3.8.rst file.

Strange. I cannot see isAlive() is https://docs.python.org/dev/library/threading.html Should we document it... just to say that it's deprecated?

Lib/threading.py Outdated Show resolved Hide resolved
@asvetlov
Copy link
Contributor

I believe isAlive() was removed from docs as a part of depreciation already.
Docs for Python 2.7 mentions the method but 3.5 doesn't.
I don't recall when the method dropped removed.
From my perspective, we don't need touching docs.

@corona10 corona10 force-pushed the bpo-35283 branch 2 times, most recently from b7002e5 to 2bca0ae Compare January 16, 2019 13:25
@corona10
Copy link
Member Author

@asvetlov @vstinner

Hi, Thank you for fast feedback!
I've updated the PR! Please take a look, please! Thanks!

vstinner
vstinner requested changes Jan 16, 2019
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

test_threading.test_old_threading_api() currently fails with -Werror:

vstinner@apu$ ./python -W error -m test -v test_threading -m test_old_threading_api
ERROR: test_old_threading_api (test.test_threading.ThreadTests)

You have to move the isAlive() call inside something like:

with self.assertWarnsRegex(DeprecationWarning, 'should be'):

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

@corona10 corona10 force-pushed the bpo-35283 branch 2 times, most recently from 35eb53b to 8a17926 Compare January 16, 2019 14:50
@corona10
Copy link
Member Author

@vstinner
I' ve updated the unit test code also support.start_threads API to use is_alive to prevent the same issue as you mentioned. (e.g test_threaded_import)

@corona10
Copy link
Member Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

vstinner
vstinner approved these changes Jan 17, 2019
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM, but can you please enhance the documentation formatting as requested? So I can merge your change.

Doc/whatsnew/3.8.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.8.rst Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Library/2019-01-07-17-17-16.bpo-35283.WClosC.rst Outdated Show resolved Hide resolved
@corona10
Copy link
Member Author

@vstinner
I 've enhanced the PR as your request!
Thanks always

@vstinner vstinner merged commit 89669ff into python:master Jan 17, 2019
@vstinner
Copy link
Member

Thanks! I merged your PR.

@corona10 corona10 deleted the bpo-35283 branch January 17, 2019 12:17
corona10 added a commit to corona10/cpython that referenced this pull request Jan 18, 2019
@corona10
bpo-35283: Add deprecation warning for Thread.isAlive (pythonGH-11454)
4bac940 
Add a deprecated warning for the threading.Thread.isAlive() method.

(cherry picked from commit 89669ff)
corona10 added a commit to corona10/cpython that referenced this pull request Jan 18, 2019
@corona10
[3.7] bpo-35283: Add deprecation warning for Thread.isAlive (pythonGH…
8a5400a 
…-11454)

Add a deprecated warning for the threading.Thread.isAlive() method..
(cherry picked from commit 89669ff)

Co-authored-by: Dong-hee Na <donghee.na92@gmail.com>
@alessiamarcolini alessiamarcolini mentioned this pull request Nov 25, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner approved these changes

@asvetlov asvetlov asvetlov approved these changes

@eamanu eamanu eamanu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@corona10 @asvetlov @bedevere-bot @vstinner @eamanu @the-knights-who-say-ni