Skip to content

Conversation

@gautamomento
Copy link
Contributor

Based on https://realpython.com/python-is-identity-vs-equality/, is not operator compares if two variables refer to the same object in memory. Hence switching to != and == comparisons. Although the types being compared are int and fall within range, it seems like the comaprison being made is incorrect.

Why only the bug reporter can reproduce this but not any of us is still a mystery to me. So unfortunately, I cannot test this fix apart from some theoretical reading that I did.

Nonetheless, this should fix it for everyone is my solid hope.

>>> import momento_wire_types.cacheclient_pb2 as cache_client_types
>>> get_resp = cache_client_types.GetResponse()
>>> type(get_resp)
<class 'cacheclient_pb2.GetResponse'>
>>> type(get_resp.result)
<class 'int'>
>>> type(cache_client_types.Hit)
<class 'int'>

Based on https://realpython.com/python-is-identity-vs-equality/, `is not` operator compares if two variables refer to the same object in memory. Hence switching to != and == comparisons. Although the types being compared are `int` and fall within range, it seems like the comaprison being made is incorrect.

Why only the bug reporter can reproduce this but not any of us is still a mystery to me. So unfortunately, I cannot test this fix apart from some theoretical reading that I did.

Nonetheless, this should fix it for everyone is my solid hope.

```
>>> import momento_wire_types.cacheclient_pb2 as cache_client_types
>>> get_resp = cache_client_types.GetResponse()
>>> type(get_resp)
<class 'cacheclient_pb2.GetResponse'>
>>> type(get_resp.result)
<class 'int'>
>>> type(cache_client_types.Hit)
<class 'int'>
```
@gautamomento
Copy link
Contributor Author

Copy link
Contributor

@bruuuuuuuce bruuuuuuuce left a comment

Choose a reason for hiding this comment

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

Do we also want to change all the other is not to != in this repo?

@gautamomento
Copy link
Contributor Author

Do we also want to change all the other is not to != in this repo?

I believe these are the only ones that compare values. The others are is not None which is the recommended way. Do you see anymore that I may have missed?

@bruuuuuuuce
Copy link
Contributor

Do we also want to change all the other is not to != in this repo?

I believe these are the only ones that compare values. The others are is not None which is the recommended way. Do you see anymore that I may have missed?

Ok cool, the rest are is not None

Copy link
Contributor

@bruuuuuuuce bruuuuuuuce left a comment

Choose a reason for hiding this comment

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

:shipit:

@gautamomento gautamomento merged commit 5d961b8 into main Jan 7, 2022
@gautamomento gautamomento deleted the u/gautamomento/rewrite-enum-comparisons branch January 7, 2022 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants