-
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
Add DiscardElements helper transformer #28
Conversation
cmp/cmpopts/transform.go
Outdated
// | ||
// DiscardMapZeros can be used in conjunction with EquateEmpty, | ||
// but cannot be used with SortMaps. | ||
func DiscardMapZeros() cmp.Option { |
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.
"zeros" and "zeroes" are both correct spellings for the plural of "zero". Perhaps it would be better to avoid that ambiguity if we can.
How about DiscardZeroElements
or OmitZeroElements
? (That could also apply to slices, e.g. for a slice of pointers with a nil pointer meaning "skip".)
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.
DiscardZeroElements also operating on slices feels like it is over-stretching what it should do. Although, I admit that it probably would not break anything since zero-values of any type is typically equal to itself. I do like the fact that I wouldn't need to think about whether this applies to maps or slices.
Let me think about this.
cmp/cmpopts/sort.go
Outdated
@@ -94,7 +94,8 @@ func (ss sliceSorter) less(v reflect.Value, i, j int) bool { | |||
// • Transitive: if !less(x, y) and !less(y, z), then !less(x, z) | |||
// • Total: if x != y, then either less(x, y) or less(y, x) | |||
// | |||
// SortMaps can be used in conjuction with EquateEmpty. | |||
// SortMaps can be used in conjunction with EquateEmpty, | |||
// but cannot be used with DiscardMapZeros. |
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.
Hmm, this is an interesting data point for our "only one option may apply" heuristic. These options happen to be commutative, so it's safe (and not particularly confusing) to apply them both.
Perhaps this is another good use-case for the "first option that applies" combiner you mentioned at GopherCon.
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.
Perhaps this is another good use-case for the "first option that applies" combiner you mentioned at GopherCon.
Yep. I was thinking the exact same thing :)
It would be really cool to be able to do something like FilterPriority(DiscardMapZeros(), SortMaps(f)) to first discard the map zeros and then sort the map.
Let's leave the comments clean-up for the CL that adds in the priority filter idea.
@alercah provided an interesting example where Consider the following: x: map[string][]int{"foo": []int{}}
y: map[string][]int{"foo": nil} If only The current API will need to be thought about more carefully. |
If the API for transformers allowed for A subtractive approach would have worked as well if it only discarded zero-values of keys that do not exist in the other map. The core problem is that Transformers were designed assuming they would never need to know about the other value. I feel like a |
Hmm, that example suggests that |
That would also be an API change where you could take in the |
I see at least 4 possible options:
Add API to allow Options to operate on "non-existent" values I am opposed to this approach since whether two elements of a slice are "non-existent" or not is dependent on the exact diffing algorithm used to compare two slices. Thus, the output of whether two objects are equal or not is now possibly dependent on the diffing algorithm, which causes the results of Add API to allow Options to allow functions to call Add API to allow Transformers to also operate on Using Although I do see some utility in Add no new API and just change how this helper is used. We can go with the following: // DiscardElements transforms all slices and maps of type []V or map[K]V,
// where type V is assignable to type T, by removing elements where the
// remove function, f, reports true.
//
// As an example, all zero elements can be discarded with:
// DiscardElements(func(v MyStruct) bool { return v == MyStruct{} })
//
// DiscardElements can be used in conjunction with EquateEmpty,
// but cannot be used with SortMaps.
func DiscardElements(f func(T) bool) cmp.Option There are two benefits to this helper options:
DiscardElements(func(v []int) bool {
return cmp.Equal(v, []int(nil), cmpopts.EquateEmpty())
}) It is a little unfortunate, that the user would have to specify |
Hmm. If you provided helper functions for the zero and zero/empty cases, I think that would be good. |
@alercah, I'm going to hold off on adding sub-helper functions for those cases. It did not seem like the original problem was common enough to warrant more helpers. We can always add those in the future. |
SGTM |
b7c0d85
to
d2e3457
Compare
PR updated with |
In some situations users want to compare two maps where missing entries are equal to those that have the zero value. For example, the following maps would be considered equal: x := map[string]int{"foo": 12345, "zero":0} y := map[string]int{"foo": 12345} To help with this, we add DiscardElements to cmpopts that transforms maps and slices by stripping entries based on a user provided function. To strip zero values, the user can provide: cmpopts.DiscardElements(func(v int) bool { return v == 0 })
I just ran into this issue in my first attempt at using cmp for some unit tests. In my particular case, one of the maps will always be a superset of the other, so I just want to set the missing keys in the smaller map to the zero value. This is what I came up with: func FillMaps(keys interface{}) cmp.Option {
var mf mapFiller
vk := reflect.ValueOf(keys)
if vk.Kind() == reflect.Map {
// Use the keys from an existing map.
mf.keyType = vk.Type().Key()
mf.keys = vk.MapKeys()
} else {
// Array or slice of keys.
mf.keyType = vk.Type().Elem()
for i, l := 0, vk.Len(); i < l; i++ {
mf.keys = append(mf.keys, vk.Index(i))
}
}
return cmp.FilterValues(mf.filter, cmp.Transformer("Fill", mf.fill))
}
type mapFiller struct {
keyType reflect.Type
keys []reflect.Value
}
func (mf mapFiller) filter(x, y interface{}) bool {
vx, vy := reflect.ValueOf(x), reflect.ValueOf(y)
return (x != nil && y != nil && vx.Type() == vy.Type()) &&
(vx.Kind() == reflect.Map && mf.keyType.AssignableTo(vx.Type().Key())) &&
(mf.hasMissingKey(vx) || mf.hasMissingKey(vy))
}
func (mf mapFiller) hasMissingKey(v reflect.Value) bool {
for _, k := range mf.keys {
if !v.MapIndex(k).IsValid() {
return true
}
}
return false
}
func (mf mapFiller) fill(m interface{}) interface{} {
vm := reflect.ValueOf(m)
if !mf.hasMissingKey(vm) {
return m
}
tm := vm.Type()
vn := reflect.MakeMap(tm)
vz := reflect.Zero(tm.Elem())
for _, k := range mf.keys {
vn.SetMapIndex(k, vz)
}
for _, k := range vm.MapKeys() {
vn.SetMapIndex(k, vm.MapIndex(k))
}
return vn.Interface()
} And I just use it like: cmp.Diff(want, got, FillMaps(got)) |
While this works for maps, it seems weird to me that the |
#121 provides the ground work for ignoring missing map and slice elements. I'm abandoning this PR since the other PR is a superior approach in my opinion. |
In some situations users want to compare two maps where missing entries
are equal to those that have the zero value.
For example, the following maps would be considered equal:
x := map[string]int{"foo": 12345, "zero":0}
y := map[string]int{"foo": 12345}
To help with this, we add DiscardElements to cmpopts that transforms maps
and slices by stripping entries based on a user provided function.
To strip zero values, the user can provide:
cmpopts.DiscardElements(func(v int) bool { return v == 0 })