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

[FR] Change cmpopts.SortSlices param from interface{} to func(a,b interface{}) bool #96

Closed
Goodwine opened this issue Oct 19, 2018 · 10 comments

Comments

@Goodwine
Copy link

I just tried using cmpopts.SortSlices, and I know.. there are no generics in Go yet, but I think the current signature is not intuitive.

Current state of the world:

opt := cmpopts.SortSlices("yay") // can do whatever I want, it's an interface{}
cmp.Diff(someFoo, otherFoo, opt)

My desired state of the world:

// Can still mess things up, but it's more obvious.
opt := cmpopts.SortSlices(func (a, b interface{}){ return a.(Foo).Val < b.(Foo).Val })
cmp.Diff(someFoo, otherFoo, opt)

This proposal goes against Issue #67 but we could make it work like...

opt := cmpopts.SortSlices(cmpopts.DefaultSort) // renamed GenericLess to DefaultSort
cmp.Diff(someFoo, otherFoo, opt)
@dsnet
Copy link
Collaborator

dsnet commented Oct 19, 2018

Hi, I'm not sure how I can get that to work though. When the signature of the input changes from func(a, b T) bool to func(a, b interface{}) bool, I no longer know what which slice types to apply the transformation to. Currently, the implementation looks at the less function to determine that it is supposed to sort []T as opposed to []R.

@dsnet
Copy link
Collaborator

dsnet commented Oct 19, 2018

With generics seriously being considered for Go2, I think it be best to just leave the API as is and see how can we make it better. There are other cases where generics would have been very helpful.

@Goodwine
Copy link
Author

The problem I have with SortSlices is that it takes a "less" function typed as interface{}, I immediately think of sort.Slice() which has the param type func(i, j int) bool, but no, this one takes func (a, b T) bool which is currently impossible to represent in Go.

I had to read a couple times the documentation on how to get this to work, and sure it does, however I think saying the func takes a param type func(a, b interface{}) bool and say that a and b have to be type asserted by the user feels like a better API.

I agree this would be nicely solved by Go2, but it sounds like that's probably happening until 2 or 3 more years.

@dsnet
Copy link
Collaborator

dsnet commented Oct 19, 2018

You didn't really address the problem I posed earlier about how func(a ,b interface{}) removes information needed to properly implement the function.

@Goodwine
Copy link
Author

I already mentioned that in "My desired state of the world" and on the previous comment that the user would be responsible for doing type assertion in this world without generics.

e.g.

func (a, b interface{}) bool { a.(Foo).Val < b.(Foo).Val }

@dsnet
Copy link
Collaborator

dsnet commented Oct 19, 2018

A desired state of the world isn't helpful if it is not implementable. As it stands, I don't think there is anything actionable here. The proposed API can't work and the other alternative is to wait for generics.

@Goodwine
Copy link
Author

What do you mean by not implementable? Should I send a PR? Maybe I'm not explaining myself properly.

Right now the API accepts an interface{} that says nothing, I can pass a string and it will compile. I know that it will fail at runtime but it's not obvious. My proposal is that for now we change the signature of SortSlices(interface{}) to SortSlices(func(a,b interface{})bool) until generics exist.

Could you clarify why is having no types better than having some types and passing the responsibility of type-asserting to the user?

@dsnet
Copy link
Collaborator

dsnet commented Oct 19, 2018

Right now the API accepts an interface{} that says nothing

interface{} says nothing to the compiler, but still says something at runtime. Reflection tells me what the type of the arguments are.

func(interface{}, interface{}) bool says a little more to the compiler, but says nothing at runtime when performing type introspection.

@dsnet
Copy link
Collaborator

dsnet commented Oct 19, 2018

See this example: https://play.golang.org/p/hcpppV3sJEL

@Goodwine
Copy link
Author

Ah... so IIUC SortSlices is not intended to be applied only to A and B in cmp.Diff(A, B), it's intended to sort any slice because if I pass a map or a struct, then each entry/property is navigated like a tree and when a slice is found if it matches the type of my less function, then the function is applied to it.

I think that's what you tried to point out on your first reply, but it wasn't that obvious. I thought SortSlices was used iff I was comparing 2 slices. Now that I get it I agree with you and waiting for generics is the best thing to do.

Thanks for the discussion 😃

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

No branches or pull requests

2 participants