-
Notifications
You must be signed in to change notification settings - Fork 95
Implement eq, ne for slice #107
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
Conversation
return True, nil | ||
} | ||
|
||
func (a *Slice) M__ne__(other Object) (Object, error) { |
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 thinking out loud, but any reason we don't implement one in terms of the other?
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 looked at the range.go and list.go implementations, so I implemented __eq__
and __ne__
in the same way.
Should I create and call a private function for the common part?
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.
not necessarily. it was just a silly question from my end.
I was just wondering whether there were a more philosophical underlying reason for this.
consistency w/ other parts of the code SGTM.
(one could also argue for reducing the number of function calls)
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.
Looking at the dict.go
and set.go
, there is some code written that calls __eq__
in __ne__
, so I called eq.
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.
looking at the dict.go code and set.go code it is actually wrong and I think your PR #110 fixes up the set case.
What you've written here is much better.
However the fact that we've made the same mistake more than once in the code base makes me think we should factor this out and in the process tighten it up more.
So how about something like this in slice, set, dict and anywhere else we make a __ne__
from an __eq__
func (a *Set) M__ne__(other Object) (Object, error) {
return NotEq(a.M__eq__(other))
}
And then put something like this in py/bool.go
// NotEq is used to invert the result of an __eq__ call
func NotEq(eq Object, err error) (Object, error) {
if err != nil {
return nil, err
}
switch eq {
case True:
return False, nil
case False:
return True, nil
case NotImplemented:
return eq, nil
}
return nil, ExceptionNewf(TypeError, "__eq__ returned a non bool: (type %s)", eq.Type().Name)
}
(all code untested!)
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.
Should I handle values other than True, False and NotImplemented?
Do we have to return an exception even if we handle it?
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.
As far as I know eq should only ever return True, False or NotImplemented but python can use any value as a truthy value so...
Perhaps more Pythonic might be this which is equivalent to using the not
operator on the result.
func NotEq(eq Object, err error) (Object, error) {
if err != nil {
return nil, err
}
if eq == NotImplemented {
return eq, nil
}
return Not(eq)
}
Do we have to return an exception even if we handle it?
We need to return the exception from the __eq__
call.
I just wrote NotEq
like that do it had a convenient way to call it NotEq(a.M__eq__(other))
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 understand. Would you please review again?
And when this PR is merged, I will modify #110 to use NotEq.
830cf76
to
4e0ab69
Compare
This looks great now - thank you. Apologies for the delay merging. |
Issue: #98