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-35429: Use raise NotImplementedError instead of NotImplemented #10999

Closed
wants to merge 3 commits into from
Closed

bpo-35429: Use raise NotImplementedError instead of NotImplemented #10999

wants to merge 3 commits into from

Conversation

rth
Copy link
Contributor

@rth rth commented Dec 6, 2018 •
edited by bedevere-bot

Closes https://bugs.python.org/issue35429

In two places in stdlib, raise NotImplemented was used instead of raise NotImplementedError. The former is not valid and produces,

>>> raise NotImplemented('message')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'NotImplementedType' object is not callable

https://bugs.python.org/issue35429

@rth rth requested a review from terryjreedy as a code owner Dec 6, 2018
@rhettinger
Copy link
Contributor

rhettinger commented Dec 6, 2018

Please add tests to prevent regression.

rth
rth commented Dec 6, 2018
View changes
Lib/test/test_ssl.py
@@ -3531,6 +3531,10 @@ def serve():
server.close()
# Sanity checks.
self.assertIsInstance(remote, ssl.SSLSocket)

with self.assertRaises(NotImplementedError):
remote.dup()
Copy link
Contributor Author

@rth rth Dec 6, 2018

Choose a reason for hiding this comment

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

Maybe this is not the best place to put this check, but it's not obvious to construct the ssl.SSLSocket instance manually, and there is no other place in this test file where we are sure to have such instance.

@rth
Copy link
Contributor Author

rth commented Dec 6, 2018

Thanks @rhettinger, added non regression tests..

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Dec 7, 2018

This is a duplicate of #10934.

@terryjreedy terryjreedy removed their request for review Dec 7, 2018
@rth rth deleted the not-implemented-error branch Dec 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
awaiting review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@rth @rhettinger @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot