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

Generics in comparison functions? #249

Open
glenjamin opened this issue Nov 24, 2022 · 4 comments · May be fixed by #255
Open

Generics in comparison functions? #249

glenjamin opened this issue Nov 24, 2022 · 4 comments · May be fixed by #255

Comments

@glenjamin
Copy link

Hello!

I was wondering if you'd considered whether it's worth it to use generics in the comparison functions, and what a migration path to that model might look like?

@dnephin
Copy link
Member

dnephin commented Nov 26, 2022

Hey Glen! I think it would make sense to start using generics for Equal and DeepEqual at some point. I'm not sure if there are any other functions that would benefit from generics. Maybe Assert and Check could use it as well.

I'm hoping it'll be a backwards compatible change. Any assertion that uses different types for Equal or DeepEqual would already be failing, so I think it might be ok to start using generics without a new major version. I've been waiting a bit for everyone to upgrade their Go version to at least 1.18 so that nothing breaks. I guess with the release of go1.20 everyone should be upgraded. We can drop Go1.17 from CI when we add Go1.20 and try using some generic arguments in Equal and DeepEqual.

I guess the signatures would look like this:

func Equal[X any](t TestingT, x X, y X, msgAndArgs ...any) { ... }

func DeepEqual[X any](t TestingT, x X, y X, opts ...gocmp.Option) { ... }

I think it's only the exported function signatures that need to change so that the compiler can check the types match. Otherwise I think all the internals probably don't need to change.

What do you think? Are there other functions that might benefit from generics? Do you know of any potential problems with changing Equal and DeepEqual without releasing a new major version?

A little while ago I added a property check that used generics here: https://github.com/gotestyourself/gotest.tools/blob/main/x/generics/property/complete.go#L60-L73, I couldn't quite get the module alias working, so I don't think it's importable yet. Once the main package is using generics I think I'll move that to x/property

@glenjamin
Copy link
Author

That's great! This pretty much aligns with what I was thinking - I think possibly it'd be nice if .Contains and .Len could be made generic, but I don't think Go's generics are flexible enough to allow string/array/slice/map in the same signature.

I don't know whether it's worth adding something like cmp.ListContains and cmp.HasKey which could be typesafe on collections and their members.

The case I ran into recently that prompted this was incorrectly comparing a struct with a pointer to the same struct type - which isn't particularly a massive pain as the diff does include the (rather subtle) &.

Given the relative stability of this package it's probably not worth using build tags to continue to provide backwards compatibility for projects using older Go versions that want to continue to update assert.

@dnephin
Copy link
Member

dnephin commented Nov 26, 2022

Nice! I like the idea of type-safe implementations like cmp.ListContains (maybe cmp.SliceContains and document that it also accepts arrays, since arrays tend to be much less common?), and cmp.HasKey ! And I guess we'd want one for cmp.StringContains as well? I also don't know if there's any way to represent a generic constraint of "is slice, map, or string".

@dnephin
Copy link
Member

dnephin commented Feb 19, 2023

I put together #255 which adds generics to the main assertions, if you'd like to check it out.

For Contains and Len, how often do you use those to check for an empty slice vs checking for a non-zero number of items? I don't use either myself, I generally do this:

// strings
assert.Assert(t, strings.Contains(actual, expected), actual)

// slices or maps
assert.DeepEqual(t, actual, expected, cmpSomething)

// slice or map empty
assert.Assert(t, len(actual), 0, "got: %v", actual)

For slices and maps, I always prefer DeepEqual instead of a Len or Contains. I'll use comparison options to limit the fields that are compared, sometimes doing a shallow comparison of only a few fields (ex: an primary key ID).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants