◐ Shell
reader mode source ↗
Skip to content

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

Merged
vstinner merged 1 commit into
python:masterfrom
corona10:bpo-35283
Jan 17, 2019
Merged

bpo-35283: Add a deprecated warning for the threading.Thread.isAlive#11454
vstinner merged 1 commit into
python:masterfrom
corona10:bpo-35283

Conversation

@corona10

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

Copy link
Copy Markdown
Member

@corona10

corona10 commented Jan 7, 2019

Copy link
Copy Markdown
Member Author

@asvetlov

Hi, Can you take a look, please?

eamanu
eamanu approved these changes Jan 7, 2019
View reviewed changes

@eamanu eamanu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

LGTM

@corona10

corona10 commented Jan 7, 2019

Copy link
Copy Markdown
Member Author

@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. Jan 7, 2019
@corona10

corona10 commented Jan 16, 2019

Copy link
Copy Markdown
Member Author

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

vstinner
vstinner reviewed Jan 16, 2019
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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?

@asvetlov

asvetlov commented Jan 16, 2019

Copy link
Copy Markdown
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

corona10 commented Jan 16, 2019

Copy link
Copy Markdown
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

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

bedevere-bot commented Jan 16, 2019

Copy link
Copy Markdown

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

corona10 commented Jan 16, 2019

Copy link
Copy Markdown
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

corona10 commented Jan 17, 2019

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again

@bedevere-bot

bedevere-bot commented Jan 17, 2019

Copy link
Copy Markdown

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

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

@corona10

corona10 commented Jan 17, 2019

Copy link
Copy Markdown
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

vstinner commented Jan 17, 2019

Copy link
Copy Markdown
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

+1 more reviewer

@eamanu eamanu eamanu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@corona10 @asvetlov @bedevere-bot @vstinner @eamanu @the-knights-who-say-ni