GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-118767: Make bool(NotImplemented) raise TypeError by JelleZijlstra · Pull Request #118775 · python/cpython

Conversation

JelleZijlstra commented May 8, 2024 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

>>> bool(NotImplemented)
Traceback (most recent call last):
  File "<python-input-0>", line 1, in <module>
    bool(NotImplemented)
    ~~~~^^^^^^^^^^^^^^^^
TypeError: NotImplemented should not be used in a boolean context

Issue: Make bool(NotImplemented) into an error #118767

📚 Documentation preview 📚: https://cpython-previews--118775.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JelleZijlstra added 4 commits

May 8, 2024 08:45


          Make bool(NotImplemented) an error

8428e3c


          Merge remote-tracking branch 'upstream/main' into boolnotimp

ee8492f


          What's New

df6a76a


          blurb

0e83fd0

bedevere-app Bot added the awaiting core review label

May 8, 2024

bedevere-app Bot mentioned this pull request

May 8, 2024

Make bool(NotImplemented) into an error #118767

Closed


          Merge branch 'main' into boolnotimp

5daaa9d

carljm approved these changes

May 8, 2024

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting core review labels

May 8, 2024

JelleZijlstra merged commit 3c079a0 into python:main

May 8, 2024

bedevere-app Bot removed the awaiting merge label

May 8, 2024

JelleZijlstra deleted the boolnotimp branch

May 8, 2024 18:12

serhiy-storchaka reviewed

May 8, 2024

View reviewed changes

Comment thread

Doc/library/constants.rst

		@@ -57,6 +57,9 @@ A small number of constants live in the built-in namespace. They are:
		it currently evaluates as true, it will emit a :exc:`DeprecationWarning`.
		It will raise a :exc:`TypeError` in a future version of Python.

serhiy-storchaka May 8, 2024

Copy link

Copy Markdown

Member

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.

This can be removed. And maybe the whole text of the note can be reworded.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JelleZijlstra May 8, 2024

Copy link

Copy Markdown

Member Author

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.

Thanks, I'll submit another PR addressing your comments.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/reference/datamodel.rst

		@@ -174,6 +174,9 @@ for more details.
		it currently evaluates as true, it will emit a :exc:`DeprecationWarning`.
		It will raise a :exc:`TypeError` in a future version of Python.

serhiy-storchaka May 8, 2024

Copy link

Copy Markdown

Member

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.

This can be removed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_builtin.py

		# a TypeError).
		self.assertWarns(DeprecationWarning, bool, NotImplemented)
		with self.assertWarns(DeprecationWarning):
		self.assertRaises(TypeError, bool, NotImplemented)

serhiy-storchaka May 8, 2024

Copy link

Copy Markdown

Member

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.

The test should be renamed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_builtin.py

		self.assertWarns(DeprecationWarning, bool, NotImplemented)
		with self.assertWarns(DeprecationWarning):
		self.assertRaises(TypeError, bool, NotImplemented)
		with self.assertRaises(TypeError):

serhiy-storchaka May 8, 2024

Copy link

Copy Markdown

Member

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.

TypeError is raised when you just call a function with wrong number of arguments, for example:

with self.assertRaises(TypeError):
    self.assertEqual(NotImplemented)

It is better to not use assertTrue() (which does not work here), but directly test the boolean value:

if NotImplemented:
    pass

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JelleZijlstra May 9, 2024

Copy link

Copy Markdown

Member Author

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'll also change it to use assertRaisesRegex to test that we're getting the expected TypeError and not some other one.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request

May 9, 2024


          pythongh-118775: Improve tests and docs for bool(NotImplemented)

46d29c4

JelleZijlstra mentioned this pull request

May 9, 2024

gh-118767: Improve tests and docs for bool(NotImplemented) #118813

Merged

serhiy-storchaka reviewed

May 9, 2024

View reviewed changes

Comment thread

Misc/NEWS.d/next/Core and Builtins/2024-05-08-09-44-15.gh-issue-118767.iFF5F5.rst

		@@ -0,0 +1,2 @@
		Using :data:`NotImplemented` in a boolean context now raises
		:exc:`TypeError`. Contributed by Jelle Zijlstra in :gh:`118767`.

serhiy-storchaka May 9, 2024

Copy link

Copy Markdown

Member

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.

Isn't the link to the issue already included in the changelog?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JelleZijlstra reacted with thumbs up emoji

All reactions

👍 1 reaction

JelleZijlstra May 9, 2024

Copy link

Copy Markdown

Member Author

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.

Thanks, fixed in the new PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Reviewers

serhiy-storchaka

serhiy-storchaka left review comments

carljm

carljm approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

JelleZijlstra commented May 8, 2024 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JelleZijlstra commented May 8, 2024 •

edited by github-actions Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.