Skip to content
This repository was archived by the owner on Nov 24, 2025. It is now read-only.

CLOSED - Comparison using is when operands support __eq__ - #3503

Closed
matbrgz wants to merge 1 commit into
apache:masterfrom
matbrgz:patch-3
Closed

matbrgz wants to merge 1 commit into
apache:masterfrom
matbrgz:patch-3

Conversation

@matbrgz

@matbrgz matbrgz commented Apr 18, 2019

Copy link
Copy Markdown

Comparison using 'is' when equivalence is not the same as identity

Comparison using 'is' when equivalence is not the same as identity
@asfgit

asfgit commented Apr 18, 2019

Copy link
Copy Markdown
Contributor

Can one of the admins verify this patch?

@ocket8888

Copy link
Copy Markdown
Contributor

That is a good change, but an interesting tidbit: in CPython this actually works properly because the interpreter will cache numeric constants and short string constants at compile time. So assigning a variable to the value of a small string actually just points it at the existing object. Since strings and numbers are immutable, this is totally safe. But it's wrong to rely on implementation quirks.

@ocket8888 ocket8888 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please fill out the Pull Request template to the best of your ability.

@matbrgz

matbrgz commented Apr 18, 2019

Copy link
Copy Markdown
Author

as nornagon says in electron/electron#17864 (comment) "Python deduplicates small integers (<= 256) so is works on them, but larger integers get their own instance and don't compare equal with is any more."

@ocket8888

Copy link
Copy Markdown
Contributor

I didn't realize there was a size limit on integers. Wonder how it handles negative integers?

@mitchell852 mitchell852 added the Traffic Ops related to Traffic Ops label Apr 18, 2019
@rawlinp

rawlinp commented Apr 18, 2019

Copy link
Copy Markdown
Contributor

I also agree with this change, and IMO this change is so simple and obvious that I'd let it bypass filling out the PR template. Next time, please fill it out though, it helps reviewers a lot.

@ocket8888

Copy link
Copy Markdown
Contributor

I disagree. One of the big things pushed for in the new template was "what versions of TC are affected by this bug?". This bug almost certainly exists in 3.0.x, and it'd be nice to have a record of that.

Most sections can easily be filled in with N/A or a short description for such a small change. Deleting the PR template is a bad habit that I'm loathe to encourage.

@matbrgz

matbrgz commented Apr 18, 2019

Copy link
Copy Markdown
Author

I Will edit adding PR template 🐶

@ocket8888

Copy link
Copy Markdown
Contributor

Obsolete as of #3691

@ocket8888 ocket8888 closed this Sep 17, 2019
@mitchell852 mitchell852 changed the title Comparison using is when operands support __eq__ CLOSED - Comparison using is when operands support __eq__ Oct 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

abandoned Traffic Ops related to Traffic Ops

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants