Skip to content

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Oct 8, 2019
Merged

Implement eq, ne for slice #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e50a59
Implement eq, ne for slice
HyeockJinKim Sep 29, 2019
80d41f0
Add tests for slice
HyeockJinKim Sep 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions py/bool.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,16 @@ func (a Bool) M__ne__(other Object) (Object, error) {
return True, nil
}

func notEq(eq Object, err error) (Object, error) {
if err != nil {
return nil, err
}
if eq == NotImplemented {
return eq, nil
}
return Not(eq)
}

// Check interface is satisfied
var _ I__bool__ = Bool(false)
var _ I__index__ = Bool(false)
Expand Down
25 changes: 25 additions & 0 deletions py/slice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,31 @@ func (r *Slice) GetIndices(length int) (start, stop, step, slicelength int, err
return
}

func (a *Slice) M__eq__(other Object) (Object, error) {
b, ok := other.(*Slice)
if !ok {
return NotImplemented, nil
}

if a.Start != b.Start {
return False, nil
}

if a.Stop != b.Stop {
return False, nil
}

if a.Step != b.Step {
return False, nil
}

return True, nil
}

func (a *Slice) M__ne__(other Object) (Object, error) {
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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)

Copy link
Contributor Author

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.

Copy link
Collaborator

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!)

Copy link
Contributor Author

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?

Copy link
Collaborator

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))

Copy link
Contributor Author

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.

return notEq(a.M__eq__(other))
}

func init() {
SliceType.Dict["start"] = &Property{
Fget: func(self Object) (Object, error) {
Expand Down
11 changes: 11 additions & 0 deletions py/tests/slice.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,15 @@
assert a.stop == 10
assert a.step == 1

assert slice(1).__eq__(slice(1))
assert slice(1) != slice(2)
assert slice(1) == slice(None, 1, None)
assert slice(0, 0, 0) == slice(0, 0, 0)

assert slice(0, 0, 1) != slice(0, 0, 0)
assert slice(0, 1, 0) != slice(0, 0, 0)
assert slice(1, 0, 0) != slice(0, 0, 0)
assert slice(0).__ne__(slice(1))
assert slice(0, None, 3).__ne__(slice(0, 0, 3))

doc="finished"