GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-122982: Prohibit bitwise inversion on bool type by Eclips4 · Pull Request #122983 · python/cpython

Conversation

@Eclips4

@Eclips4 Eclips4 commented Aug 13, 2024 •
edited by github-actions Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: Extend deprecation warning period for bitwise inversion on bool type #122982

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Eclips4
Prohibit bitwise inversion on bool
75348d6
@Eclips4
Eclips4 requested a review from JelleZijlstra August 13, 2024 18:46
@bedevere-app bedevere-app Bot mentioned this pull request Aug 13, 2024
Extend deprecation warning period for bitwise inversion on bool type #122982
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Aug 13, 2024
@Eclips4
Fix blurb
8dc66a7
sobolevn
sobolevn approved these changes Aug 13, 2024
View reviewed changes

@sobolevn sobolevn left a comment

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.

Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Aug 13, 2024
@JelleZijlstra
JelleZijlstra removed their request for review August 13, 2024 20:31
picnixz
picnixz reviewed Aug 14, 2024
View reviewed changes
Comment thread Misc/NEWS.d/next/Core and Builtins/2024-08-13-21-40-20.gh-issue-122982.JSLp1k.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/boolobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_bool.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Eclips4 @picnixz
Apply suggestions from code review
77b4080 
Co-authored-by: Bénédikt Tran <[email protected]>
picnixz
picnixz reviewed Aug 14, 2024
View reviewed changes
Comment thread Misc/NEWS.d/next/Core and Builtins/2024-08-13-21-40-20.gh-issue-122982.JSLp1k.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Doc/whatsnew/3.14.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Eclips4 added 3 commits August 14, 2024 11:37
@Eclips4
Add more test cases
a8fdf66
@Eclips4
func -> class
7a3e73a
@Eclips4
Reword news
9bf6fdf
@Eclips4
Eclips4 requested review from AA-Turner and picnixz August 14, 2024 09:44
sobolevn
sobolevn reviewed Aug 14, 2024
View reviewed changes
Comment thread Lib/test/test_bool.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
picnixz
picnixz approved these changes Aug 14, 2024
View reviewed changes

@picnixz picnixz left a comment

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.

Apart from Nikita's comment, this looks good to me now (TIL I cannot do class A(bool): ...).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
encukou
encukou previously requested changes Aug 14, 2024
View reviewed changes

@encukou encukou left a comment •
edited
Loading

Uh oh!

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

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.

If you do this, you should also want to edit Doc/deprecations/pending-removal-in-future.rst.
But note that it's not in Doc/deprecations/pending-removal-in-3.14.rst; removing this feature now would be a surprise for some.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting merge labels Aug 14, 2024
@bedevere-app

bedevere-app Bot commented Aug 14, 2024

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Eclips4 added 2 commits August 14, 2024 14:05
@Eclips4
Address Nikita's comment
33b1397
@Eclips4
Address Petr's comment
4760de8
@Eclips4

Eclips4 commented Aug 14, 2024

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app bedevere-app Bot added awaiting change review and removed awaiting changes labels Aug 14, 2024
@bedevere-app

bedevere-app Bot commented Aug 14, 2024

Copy link
Copy Markdown

Thanks for making the requested changes!

@encukou, @sobolevn: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-app
bedevere-app Bot requested a review from encukou August 14, 2024 18:59
@bedevere-app
bedevere-app Bot requested a review from sobolevn August 14, 2024 18:59
AA-Turner
AA-Turner reviewed Aug 14, 2024
View reviewed changes
Comment thread Doc/deprecations/pending-removal-in-future.rst

* :mod:`builtins`:

* ``~bool``, bitwise inversion on bool.

@AA-Turner AA-Turner Aug 14, 2024 •
edited
Loading

Uh oh!

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

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.

If this gets merged, you should open a new PR to move this note to the 3.14 pending removals & backport that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Eclips4 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
serhiy-storchaka
serhiy-storchaka reviewed Aug 15, 2024
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

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.

Just wondering: what if simply set PyBool_Type.tp_as_number->nb_invert = NULL in _PyTypes_InitTypes()?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Eclips4

Eclips4 commented Aug 15, 2024

Copy link
Copy Markdown
Member Author

Just wondering: what if simply set PyBool_Type.tp_as_number->nb_invert = NULL in _PyTypes_InitTypes()?

Yeah, this works. Do you think it's worth doing or should we continue with the current approach?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@serhiy-storchaka

serhiy-storchaka commented Aug 16, 2024

Copy link
Copy Markdown
Member

I do not know. How does True.__inv__() work in this case?

On one hand, using the same code to generate error messages guarantees that there is no difference in error messages. On other hand, we may want to have a different error message, for example add a suggestion about not.
All reactions

Sorry, something went wrong.

Uh oh!

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

Eclips4 commented Aug 16, 2024

Copy link
Copy Markdown
Member Author

I do not know. How does True.__inv__() work in this case?

bool.__invert__ would work in this case:

Python 3.14.0a0 (heads/remove_bool_invert-dirty:4760de8ecbb, Aug 16 2024, 09:24:28) [Clang 15.0.0 (clang-1500.3.9.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> True.__invert__()
-2

So, this is a wrong approach.

On other hand, we may want to have a different error message, for example add a suggestion about not.

That's sounds good. I'll add a suggestion and tests for it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Eclips4
Add a suggestion
a949b8a
@Eclips4
Eclips4 requested a review from serhiy-storchaka August 16, 2024 06:39
@Eclips4
Add few test cases
0e26ff8
serhiy-storchaka
serhiy-storchaka reviewed Aug 16, 2024
View reviewed changes
Comment thread Lib/test/test_bool.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Eclips4
Use raw strings
d0d8ffa
serhiy-storchaka
serhiy-storchaka approved these changes Aug 16, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting change review labels Aug 16, 2024
@encukou
encukou dismissed their stale review August 16, 2024 07:11

pending-removal-in-future.rst is fixed

kumaraditya303
kumaraditya303 approved these changes Aug 17, 2024
View reviewed changes
sobolevn
sobolevn approved these changes Aug 17, 2024
View reviewed changes

@sobolevn sobolevn left a comment

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.

Thank you! I don't click merge, because @encukou's comment here: #122983 (review) seems like it is better to wait for resolution.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
AA-Turner
AA-Turner approved these changes Aug 17, 2024
View reviewed changes

@AA-Turner AA-Turner left a comment

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.

Wordsmithing: We seem to use "Maybe you meant" for SyntaxErrors, with "Did you mean" used more generally. Non-blocking suggestion though.

A

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Eclips4 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Objects/boolobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_bool.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Eclips4 @AA-Turner
Apply suggestions from code review
d980ef2 
Co-authored-by: Adam Turner <[email protected]>
hauntsaninja
hauntsaninja reviewed Aug 24, 2024
View reviewed changes

@hauntsaninja hauntsaninja left a comment

Copy link
Copy Markdown
Contributor

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 mentioned this on the issue, but I think in this case we should extend the deprecation period beyond the minimum #122982 (comment)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/boolobject.c
return PyLong_Type.tp_as_number->nb_invert(v);
PyErr_SetString(
PyExc_TypeError,
"bad operand type for unary ~: 'bool'. Did you mean 'not' instead of '~'?"

@hauntsaninja hauntsaninja Aug 24, 2024

Copy link
Copy Markdown
Contributor

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 think the text in the warning was helpful, especially since the previous behaviour has been around forever.

Maybe something like:

bad operand type for unary ~: 'bool'. Did you mean 'not' instead of '~'? Use 'not x' for boolean negation and '~int(x)` in the rare case you want bitwise inversion of the underlying int"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AA-Turner reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@Eclips4

Eclips4 commented Aug 25, 2024

Copy link
Copy Markdown
Member Author

Closing per #122982 (comment)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Eclips4 Eclips4 closed this Aug 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hauntsaninja hauntsaninja hauntsaninja left review comments
@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes
@sobolevn sobolevn sobolevn approved these changes
@AA-Turner AA-Turner AA-Turner approved these changes
@picnixz picnixz picnixz approved these changes
@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes
@encukou encukou Awaiting requested review from encukou

Uh oh!

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

Assignees

No one assigned

Labels

awaiting merge

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.

8 participants

@Eclips4 @serhiy-storchaka @encukou @sobolevn @AA-Turner @picnixz @hauntsaninja @kumaraditya303
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.