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

Fix uint to format in decimal #199

Merged
merged 10 commits into from
May 15, 2020
Merged

Fix uint to format in decimal #199

merged 10 commits into from
May 15, 2020

Conversation

178inaba
Copy link
Contributor

Since byte is an alias for uint8, format it in hex.

Fixes #185.

@dsnet
Copy link
Collaborator

dsnet commented May 13, 2020

Thanks for the PR. I feel like we should always use decimal except for []uint8. This is different from what this PR does, which is to just special-case uint8 by itself. It's still reasonable to use hexadecimal for uintptr.

@178inaba
Copy link
Contributor Author

@dsnet Thanks for the review!
I found the case of isBinary.

go-cmp/cmp/report_slices.go

Lines 132 to 147 in 049b73f

// If the text appears to be binary data,
// then perform differencing in approximately fixed-sized chunks.
// The output is inspired by hexdump.
case isBinary:
list = opts.formatDiffSlice(
reflect.ValueOf(sx), reflect.ValueOf(sy), 16, "byte",
func(v reflect.Value, d diffMode) textRecord {
var ss []string
for i := 0; i < v.Len(); i++ {
ss = append(ss, formatHex(v.Index(i).Uint()))
}
s := strings.Join(ss, ", ")
comment := commentString(fmt.Sprintf("%c|%v|", d, formatASCII(v.String())))
return textRecord{Diff: d, Value: textLine(s), Comment: comment}
},
)

This seems to be the case when it is []byte and not unicode.

case t.Kind() == reflect.Slice && t.Elem() == reflect.TypeOf(byte(0)):

if !(unicode.IsPrint(r) || unicode.IsSpace(r)) || r == utf8.RuneError {

Should I format in hex only for isBinary and uintptr?

Added isSlice as an argument of FormatValue.
If the value is uint8 and isSlice is true, FormatValue formats the value in hex.
@178inaba
Copy link
Contributor Author

178inaba commented May 14, 2020

@dsnet Hello!
I wrote the code to format only []uint8 type in hex.
Would you please review this pull request again?

Copy link
Collaborator

@dsnet dsnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. Minor nits.

@@ -81,14 +81,20 @@ func (opts formatOptions) FormatDiff(v *valueNode) textNode {
return opts.FormatDiffSlice(v)
}

var isSlice bool
if v.parent != nil &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Stye-nit): Keep conditional as single line.

@@ -81,14 +81,20 @@ func (opts formatOptions) FormatDiff(v *valueNode) textNode {
return opts.FormatDiffSlice(v)
}

var isSlice bool
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps call this withinSlice instead (here and elsewhere)? Since we're usually looking at this variable from the perspective of the child.

@178inaba 178inaba requested a review from dsnet May 15, 2020 02:25
@178inaba
Copy link
Contributor Author

@dsnet Thank you for the review!
I fixed it!

@dsnet
Copy link
Collaborator

dsnet commented May 15, 2020

Thanks!

@dsnet dsnet closed this May 15, 2020
@dsnet dsnet reopened this May 15, 2020
@dsnet
Copy link
Collaborator

dsnet commented May 15, 2020

Ooops. Forgot to merge it.

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

Successfully merging this pull request may close these issues.

print uint's in decimal?
2 participants