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

use go-cmp instead of reflect.DeepEqual #99

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mekegi
Copy link
Contributor

@mekegi mekegi commented May 13, 2019

#98

https://github.com/google/go-cmp/ is more usefull then reflect.DeepEqual

I changed reflect.DeepEqual to cmp.Equal
and add to t.Errorf cmp.Diff for more detailed view of diff between got and want
for exmaple how it looks like

=== RUN   TestAddress2addressResultSlice/two_diff_addr
    --- FAIL: TestAddress2addressResultSlice/two_diff_addr (0.00s)
        helpers_test.go:161: Address2addressResultSlice() = [address:<UID:"ololo1" >  address:<UID:"trololo" > ], want [address:<UID:"ololo" >  address:<UID:"trololo" > ]
             diff =   []*address_api.AddressResult{
              	&{
              		Precision:            s"other",
            - 		Address:              s`UID:"ololo1" `,
            + 		Address:              s`UID:"ololo" `,
              		Distance:             0,
              		... // 2 identical fields
              	},
              	&{Address: s`UID:"trololo" `},
              }
FAIL

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 96.411% when pulling 3cb5a24 on mekegi:go-cmp into afa0a37 on cweill:develop.

@coveralls
Copy link

coveralls commented May 13, 2019

Coverage Status

Coverage increased (+0.05%) to 96.411% when pulling cec4af4 on mekegi:go-cmp into afa0a37 on cweill:develop.

@cweill
Copy link
Owner

cweill commented Jun 30, 2019

@mekegi: This looks good to me, except can you please add a flag use_go_cmp=true. That way people who want the old behavior (minus a dependency) can use it.

@smitt04
Copy link

smitt04 commented Jun 5, 2020

I see it has been a year since this PR was created. I would love to use cmp instead of reflect. Is going to get merged or is it dead?

@sljeff
Copy link

sljeff commented Jun 19, 2020

+1

Now gopls checks DeepEqual with errors.

17:03:20 ➜ gopls check app/grpc/handlers_test.go
/Users/jeff/proj/learning/app/grpc/handlers_test.go:2073:9-49: avoid using reflect.DeepEqual with errors
/Users/jeff/proj/learning/app/grpc/handlers_test.go:3163:7-55: avoid using reflect.DeepEqual with errors
/Users/jeff/proj/learning/app/grpc/handlers_test.go:4498:9-49: avoid using reflect.DeepEqual with errors
/Users/jeff/proj/learning/app/grpc/handlers_test.go:5076:9-49: avoid using reflect.DeepEqual with errors
/Users/jeff/proj/learning/app/grpc/handlers_test.go:5291:9-49: avoid using reflect.DeepEqual with errors

@cweill
Copy link
Owner

cweill commented Dec 31, 2020

If OP or someone else can update this PR to resolve branch conflicts, I will approve it.

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

Successfully merging this pull request may close these issues.

5 participants