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.

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-45735: Promise the long-time truth that args=list works #30982

Merged
merged 15 commits into from Feb 26, 2022
Merged

bpo-45735: Promise the long-time truth that args=list works #30982

merged 15 commits into from Feb 26, 2022

Conversation

CharlieZhao95
Copy link
Contributor

@CharlieZhao95 CharlieZhao95 commented Jan 28, 2022 •
edited by bedevere-bot

Change description of threading.Thread to explain the validity of list args.

Add doc example and test cases for threading.Thread.

https://bugs.python.org/issue45735

tim-one
tim-one reviewed Jan 28, 2022
View changes
Copy link
Member

@tim-one tim-one left a comment

So far, so good! Similar changes are needed for multiprocessing.Process (in file multiprocessing.rst), for its args argument.

@tim-one tim-one requested a review from rhettinger Jan 28, 2022
@tim-one
Copy link
Member

tim-one commented Jan 28, 2022

@rhettinger, you closed your old PR on this without leaving a clue as to why. If you object to documenting this now, please say so.

tim-one
tim-one reviewed Jan 28, 2022
View changes
Copy link
Member

@tim-one tim-one left a comment

You should also add your name to Misc/ACKS.

@CharlieZhao95
Copy link
Contributor Author

CharlieZhao95 commented Jan 29, 2022

So far, so good! Similar changes are needed for multiprocessing.Process (in file multiprocessing.rst), for its args argument.

OK, I am going to submit the relevant code on this PR (Previously, I plan to open a new PR for multiprocessing ).😉

shihai1991
shihai1991 reviewed Jan 29, 2022
View changes
Lib/test/test_threading.py Outdated Show resolved Hide resolved
tim-one
tim-one requested changes Jan 29, 2022
View changes
Copy link
Member

@tim-one tim-one left a comment

Looking good! I suggested some changes for clearer English. You should be able to just a click a button to accept them, if you agree with them.

Doc/library/multiprocessing.rst Outdated Show resolved Hide resolved
Doc/library/multiprocessing.rst Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

bedevere-bot commented Jan 29, 2022

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.

@tim-one tim-one changed the title bpo-45735: Promised that lists can be used for Thread args. bpo-45735: Promise the long-time truth that args=list works Jan 29, 2022
merwok
merwok reviewed Jan 29, 2022
View changes
Lib/test/_test_multiprocessing.py Outdated Show resolved Hide resolved
CharlieZhao95 and others added 2 commits Jan 29, 2022
@CharlieZhao95 @tim-one
Update Doc/library/multiprocessing.rst
d641380 
Cleaner English here.

Co-authored-by: Tim Peters <tim.peters@gmail.com>
@CharlieZhao95 @tim-one
Update Doc/library/multiprocessing.rst
547652d 
Cleaner English to describe *args*.

Co-authored-by: Tim Peters <tim.peters@gmail.com>
merwok
merwok reviewed Jan 30, 2022
View changes
Lib/test/test_threading.py Outdated Show resolved Hide resolved
@CharlieZhao95 CharlieZhao95 requested a review from tim-one Feb 1, 2022
@CharlieZhao95
Copy link
Contributor Author

CharlieZhao95 commented Feb 10, 2022

I have made the requested changes; please review again

@bedevere-bot
Copy link

bedevere-bot commented Feb 10, 2022

Thanks for making the requested changes!

@tim-one: please review the changes made to this pull request.

shihai1991
shihai1991 approved these changes Feb 11, 2022
View changes
Copy link
Member

@shihai1991 shihai1991 left a comment

Thanks, LGTM.

Doc/library/multiprocessing.rst Outdated Show resolved Hide resolved
@tim-one tim-one merged commit e466faa into python:main Feb 26, 2022
13 checks passed
asvetlov pushed a commit that referenced this pull request Feb 26, 2022
@CharlieZhao95 @asvetlov
bpo-45735: Promise the long-time truth that args=list works (GH-30982)
708e026 
For threads, and for multiprocessing, it's always been the case that ``args=list`` works fine when passed to ``Process()`` or ``Thread()``, and such code is common in the wild. But, according to the docs, only a tuple can be used. This brings the docs into synch with reality.

Doc changes by Charlie Zhao.
Co-authored-by: Tim Peters <tim.peters@gmail.com>
@tim-one tim-one mentioned this pull request Apr 11, 2022
Closed
@vstinner
Copy link
Member

vstinner commented May 17, 2022

The added test leaks threads running in the background: I wrote #92885 to fix this issue.

@tim-one
Copy link
Member

tim-one commented May 17, 2022

Thank you, Victor! 😄

1 similar comment
@tim-one
Copy link
Member

tim-one commented May 17, 2022

Thank you, Victor! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@merwok merwok

@shihai1991 shihai1991

@tim-one tim-one

@rhettinger rhettinger

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

Successfully merging this pull request may close these issues.

None yet
7 participants
@CharlieZhao95 @tim-one @bedevere-bot @vstinner @merwok @shihai1991 @the-knights-who-say-ni