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

Create a simple example for the IgnoreFields #205

Merged
merged 5 commits into from Oct 4, 2020

Conversation

@colinnewell
Copy link
Contributor

@colinnewell colinnewell commented May 31, 2020

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.

@frioux
Copy link

@frioux frioux commented Oct 3, 2020

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.

@colinnewell
Copy link
Contributor Author

@colinnewell colinnewell commented Oct 3, 2020

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 cmp/example_test.go which seemed ugly. The other thing that I'm struggling to figure out is why that one doesn't suffer from the same problem mine does.

If figure I may as well try to get the tests to pass consistently rather than randomly.

@colinnewell colinnewell force-pushed the ignorefields-example branch from 3bf9162 to 86c02c9 Oct 3, 2020
@colinnewell
Copy link
Contributor Author

@colinnewell colinnewell commented Oct 3, 2020

Ah, found it: cmp/report_text.go.

@colinnewell
Copy link
Contributor Author

@colinnewell colinnewell commented Oct 3, 2020

Thanks for the prod. I've rebased and fixed the test so that it runs consistently.

Copy link
Member

@dsnet dsnet left a comment

Thanks!

// and want be the expected golden data.
got, want := MakeGatewayInfo()

// Note that the Diff will still potentially show ignored fields as different,
Copy link
Member

@dsnet dsnet Oct 3, 2020

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.

Copy link
Member

@dsnet dsnet Oct 3, 2020

// 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
}

Copy link
Contributor Author

@colinnewell colinnewell Oct 4, 2020

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?

Copy link
Member

@dsnet dsnet Oct 4, 2020

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
Copy link
Member

@dsnet dsnet Oct 3, 2020

Please add license header.

Copy link
Contributor Author

@colinnewell colinnewell Oct 4, 2020

Done

Copy link
Member

@dsnet dsnet Oct 4, 2020

I know people aren't fond of 2020, but that's the current year ;)

cmp/cmpopts/example_test.go Outdated Show resolved Hide resolved
cmp/cmpopts/example_test.go Outdated Show resolved Hide resolved
@@ -0,0 +1,125 @@
package cmpopts_test
Copy link
Member

@dsnet dsnet Oct 4, 2020

I know people aren't fond of 2020, but that's the current year ;)

// and want be the expected golden data.
got, want := MakeGatewayInfo()

// Note that the Diff will still potentially show ignored fields as different,
Copy link
Member

@dsnet dsnet Oct 4, 2020

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.

}

// Use IgnoreFields to ignore fields on a type when comparing.
// Provide an interface of the type and the field names to ignore.
Copy link
Member

@dsnet dsnet Oct 4, 2020

// 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{}).

Copy link
Contributor Author

@colinnewell colinnewell Oct 4, 2020

Used, thanks.

colinnewell added 2 commits Oct 4, 2020
Updated copyright year and used clearer language to explain differences
showing up.
@dsnet dsnet merged commit 566225a into google:master Oct 4, 2020
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants