-
Notifications
You must be signed in to change notification settings - Fork 209
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
Change diff.Difference to always return an edit-script #45
Conversation
ffd02af
to
f07c4cc
Compare
cmp/compare.go
Outdated
// Report the entire slice as is if the arrays are of primitive kind, | ||
// and the arrays are different enough. | ||
k := t.Elem().Kind() | ||
isPrimitive := reflect.Bool <= k && k <= reflect.Complex128 |
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.
Relying on the order of constants in the reflect package doesn't seem like a great idea.
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
f07c4cc
to
f8cbfe4
Compare
Rather than performing the heuristic for whether two slices are "too different" in the diff package, just return the full edit-script and move the heuristic logic into the cmp package. The main adjustment to the heuristic is that we only print the full slice if it is composed of a slice of primitive types (e.g., []byte) and the difference between the two slices is sufficiently great enough. Fixes #44
f4b3429
to
ee204b6
Compare
reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr, | ||
reflect.Bool, reflect.Float32, reflect.Float64, reflect.Complex64, reflect.Complex128: | ||
isPrimitive = true | ||
} |
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.
Personally, I'd hoist isPrimitive into a function of its own. Up to you.
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'm going to leave it as is for now. The sequence of grouping all of the Ints and Uints together seems to appear very often in the code:
$ rgrep "reflect\.(Int|Uint),"
compare.go:216: case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
compare.go:219: case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
compare.go:389: case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64,
compare.go:390: reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr,
internal/value/sort.go:47: case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
internal/value/sort.go:49: case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
internal/value/format.go:52: case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
internal/value/format.go:54: case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
internal/value/format.go:219: case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
internal/value/format.go:221: case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
I may just make a helper function to quickly identify whether something is an Int or Uint regardless of size.
Rather than performing the heuristic for whether two slices are
"too different" in the diff package, just return the full edit-script
and move the heuristic logic into the cmp package.
The main adjustment to the heuristic is that we only print the
full slice if it is composed of a slice of primitive types (e.g., []byte)
and the difference between the two slices is sufficiently great enough.
Fixes #44