Skip to content

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

Closed
rth wants to merge 3 commits intopython:masterfrom
rth:not-implemented-error
Closed

bpo-35429: Use raise NotImplementedError instead of NotImplemented#10999
rth wants to merge 3 commits intopython:masterfrom
rth:not-implemented-error

Conversation

@rth
Copy link
Copy Markdown
Contributor

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

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 December 6, 2018 20:08
@rhettinger
Copy link
Copy Markdown
Contributor

rhettinger commented Dec 6, 2018

Please add tests to prevent regression.

rth
rth commented Dec 6, 2018
View reviewed changes
Lib/test/test_ssl.py
self.assertIsInstance(remote, ssl.SSLSocket)

with self.assertRaises(NotImplementedError):
remote.dup()
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

rth commented Dec 6, 2018

Thanks @rhettinger, added non regression tests..

@serhiy-storchaka
Copy link
Copy Markdown
Member

serhiy-storchaka commented Dec 7, 2018

This is a duplicate of #10934.

@terryjreedy terryjreedy removed their request for review December 7, 2018 06:34
@rth rth deleted the not-implemented-error branch December 7, 2018 07:07
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.

5 participants

@rth @rhettinger @serhiy-storchaka @the-knights-who-say-ni @bedevere-bot