-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
bpo-32208: update threading.Semaphore docs and add unit test #4709
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
Conversation
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. Thanks again to your contribution and we look forward to looking at it! |
…validate correct behavior
During testing I also found an exception with the language "returns nothing" when it actually returns True. |
the PSF should be signed! |
I'm not sure what to do for the rest of the issues. I got this in the travis build:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed docs formatting but test should be improved
Lib/test/test_threading.py
Outdated
|
||
start = time.time() | ||
while th1.is_alive(): | ||
assert time.time() - start < 0.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use unittest methods like self.assertEqual()
and family: https://docs.python.org/3/library/unittest.html#assert-methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are tests for semaphores in BaseSemaphoreTests
in Lib/test/lock_tests.py
. You should look there and add any tests there as needed instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(but everything seems to be tested already there, except the return value of blocking semaphores which wasn't really documented anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see those, thanks for pointing it out.
I've removed the "doc tests" and added testing the return type to the base semaphore tests. Let me know if that looks good!
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 |
…ue' test to BaseSemaphore
Sorry, I don't see your card in https://bugs.python.org/user?username=&realname=&github=vitiral&%40action=search |
@asvetlov I forgot to put in my "github user name". Should be in there now! |
GH-4750 is a backport of this pull request to the 3.6 branch. |
Sorry, @vitiral and @asvetlov, I could not cleanly backport this to |
Docs for 2.7 is slightly different, let's not add the change to Python 2.7 |
@asvetlov the only difference between them should be the addition of However, I am fine leaving python2.7 (including its docs) alone since it really isn't relevant anymore. The only annoyance is that google STILL points to python2.7 as the "main docs", but meh. |
and thanks @miss-islington and @asvetlov, it was a great first contribution for a language I've been using for over half a decade now 🎉 |
@vitiral docs for 2.7 has a little different paragraphs flow but the fix is not very important to make docs divergency. |
@asvetlov (sorry if this shows up in your email twice, I replied to the wrong thread before!) The 2.7 docs has one error: It says "There is no return value in this case" for when a thread is blocked, when in fact the return value is I don't think a port would be difficult if you would be open to getting it into 2.7 (Edit: I can work on it). |
This attempts to fix https://bugs.python.org/issue32208
https://bugs.python.org/issue32208