-
Notifications
You must be signed in to change notification settings - Fork 208
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
Create a simple example for the IgnoreFields #205
Conversation
This seems like a solid PR to me. The only problem is that cmp explicitly thwarts tests like this by injecting randomness into the output format. @colinnewell you might set the flag that disables that for your test and then it'd pass. |
Thanks for the comments @frioux. Did you see where this randomness is coming from? I'm failing to spot it so far. One of the reasons I didn't push this PR too far was that it was pretty much a rip off of If figure I may as well try to get the tests to pass consistently rather than randomly. |
3bf9162
to
86c02c9
Compare
Ah, found it: |
Thanks for the prod. I've rebased and fixed the test so that it runs consistently. |
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.
Thanks!
cmp/cmpopts/example_test.go
Outdated
// and want be the expected golden data. | ||
got, want := MakeGatewayInfo() | ||
|
||
// Note that the Diff will still potentially show ignored fields as different, |
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.
will still potentially show ignored fields as different,
Is still true? I recall fixing the reporter to stop printing ignored fields and using ...
instead, even for adjacent fields.
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.
Lines 258 to 265 in d713870
// Elide ignored nodes. | |
if r.Value.NumIgnored > 0 && r.Value.NumSame+r.Value.NumDiff == 0 { | |
deferredEllipsis = !(k == reflect.Slice || k == reflect.Array) | |
if !deferredEllipsis { | |
list.AppendEllipsis(diffStats{}) | |
} | |
continue | |
} |
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.
The example itself demonstrates what I was trying to say. I ignore IPAddress
but it still shows up in the diff. I assume that the reason I added 'potentially' to the sentence is because I had come across your behaviour to minimise that.
If you do end up changing that behaviour, the example will highlight it so it should be easy to change that comment then.
Alternatively, is it the word 'adjacent' that you're objecting to?
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 think there's a crucial difference between "in the diff" as you just said versus "as different" as currently written in the example. The former denotes an issues with display representation, while the latter subtly suggests something about the semantics of how the comparison is performed.
Perhaps this is better worded as:
// While the specified fields will be semantically ignored for the comparison,
// the fields may be printed in the diff when displaying entire values
// that are already determined to be different.
@@ -0,0 +1,125 @@ | |||
package cmpopts_test |
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.
Please add license header.
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.
Done
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 know people aren't fond of 2020, but that's the current year ;)
@@ -0,0 +1,125 @@ | |||
package cmpopts_test |
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 know people aren't fond of 2020, but that's the current year ;)
cmp/cmpopts/example_test.go
Outdated
// and want be the expected golden data. | ||
got, want := MakeGatewayInfo() | ||
|
||
// Note that the Diff will still potentially show ignored fields as different, |
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 think there's a crucial difference between "in the diff" as you just said versus "as different" as currently written in the example. The former denotes an issues with display representation, while the latter subtly suggests something about the semantics of how the comparison is performed.
Perhaps this is better worded as:
// While the specified fields will be semantically ignored for the comparison,
// the fields may be printed in the diff when displaying entire values
// that are already determined to be different.
cmp/cmpopts/example_test.go
Outdated
} | ||
|
||
// Use IgnoreFields to ignore fields on a type when comparing. | ||
// Provide an interface of the type and the field names to ignore. |
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.
// Use IgnoreFields to ignore fields on a struct type when comparing
// by providing a value of the type and the field names to ignore.
// Typically, a zero value of the type is used (e.g., foo.MyStruct{}).
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.
Used, thanks.
Updated copyright year and used clearer language to explain differences showing up.
I was struggling to figure out how to use IgnoreFields initially so I figured an example would be helpful. I've essentially ripped off the example from the parent module with a proper copy paste job for now with a quick example.
If you can suggest a better way to do it, I'd be happy to clean up the code.
I suspect I could also make the example clearer, but I'd quite like to clean up the code before going too far.