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

Skip Eq and Is replacement #162

Merged
merged 1 commit into from Jul 26, 2016

Conversation

@atodorov
Copy link
Contributor

commented Jul 25, 2016

In my experience these two appear to be equivalent and it makes sense to skip this replacement in the same fashion we do for their Not counterparts.

@abingham what do you think ?

@atodorov atodorov force-pushed the MrSenko:operator_skip branch from e624f2a to 6abe4de Jul 25, 2016

@abingham

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2016

(Sorry, this turned into a bit of a "think out loud" session)

I had thought about this one earlier, but I wasn't convinced that == and is are equivalent enough. Fundamentally, they're very different things, and - unlike the existing !=/is not exception - is is not an almost perfect superset of ==.

(I can think of only one common case where is not is not the same as !=:

>>> x = float('NaN')
>>> x != x
True
>>> x is not x
False

While you technically can create classes that exhibit this behavior, it's such an oddity that many programmers have to be continually reminded that NaNs are never equal. Which is to say that I think != and is not are equivalent enough for our purposes right now.)

On the other hand, I can think of a vast number of examples where is would not have the same result as ==. Any class that defines __eq__ in a conventional manner would likely break this equivalence. This probably is an argument for adding an exception for is/== replacement...things that are the same are generally equal.

Where things get really painful is with e.g. numbers. Consider:

>>> x = 1
>>> y = 1
>>> y == x
True
>>> y is x
True
>>> a = 12345678
>>> b = 12345678
>>> a == b
True
>>> a is b
False
>>>

These kinds of relationships are implementation-specific, probably version-specific, not promised or documented, and possibly dynamic. So no matter what strategy we choose WRT exceptions, we're in a hole.

Ultimately, I think the skip set is an interim solution. We'll really solve this with a robust exceptions system that lets users indicate when to avoid certain operators. We might also broaden the operator API e.g. by letting operator developers add metadata which could be filtered on. For instance, certain operators could be marked "often equivalent" or something, and users could choose to not apply those at all.

@abingham abingham merged commit 5d10929 into sixty-north:master Jul 26, 2016

2 checks passed

code-quality/landscape Code quality remained the same
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abingham

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2016

All of which is to say, I'll take this patch since it's probably really convenient for you and other users right now. I'm going to add a note to the skip set, though, indicating that it's an imperfect interim solution. Thanks for all of your work on this. It's really is great to get collaborators!

@atodorov atodorov deleted the MrSenko:operator_skip branch Oct 5, 2016

@atodorov atodorov restored the MrSenko:operator_skip branch Oct 5, 2016

@atodorov atodorov deleted the MrSenko:operator_skip branch Oct 5, 2016

@proggga

This comment has been minimized.

Copy link

commented May 15, 2017

I think we should add some exclusions
I have example, which is really valid for test,
because logic doesn't changed, but I don't know, how it can be fixed:
i have comprehension like:

[i for i in list if i is not None]

cosmic rays change it to

[i for i in list if i != None]

and my test doesn't fail, how I should pass this situation

@abingham

This comment has been minimized.

Copy link
Contributor

commented May 15, 2017

@proggga I agree that this mutation would be difficult for tests for detect, so another temporary exception is in order. Can you put together a PR for that?

If possible, I'd like the exception to be as tight as possible, only preventing mutation when None is one of the arguments to is not. I think this is possible, but I might be wrong.

@proggga

This comment has been minimized.

Copy link

commented May 15, 2017

I'm sorry, but what you mean under "put together a PR", I will be happy to help with this project

@abingham

This comment has been minimized.

Copy link
Contributor

commented May 15, 2017

That just means to put create a Pull Request with the appropriate changes. We'll look them over, maybe requests changes, and then merge it in. You can read more about PRs in the github documentation.

(If you already know all of this, I don't mean to be rude or anything...I'm just not sure how much you know yet. I'll be happy to help you through the process if you need.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.