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

FFM-2934 Bugfix - compare custom attributes based on string values #78

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

jcox250
Copy link
Contributor

@jcox250 jcox250 commented Apr 5, 2022

It turns out attrValue.String() will not return a string value if attrValue is an int, boolean or any other non string type. So I've made a change to check the Kind of attrValue and then do the appropriate conversion for that kind to a string.

reflect.Invalid, reflect.Ptr, reflect.Slice, reflect.Struct, reflect.Uintptr, reflect.UnsafePointer,
reflect.Float32, reflect.Float64, reflect.Int16, reflect.Int32, reflect.Int8, reflect.Map, reflect.Uint,
reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uint8:
object = fmt.Sprintf("%v", object)
Copy link
Contributor

Choose a reason for hiding this comment

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

this will be slower than conv, Sprintf accept interface and it will do one pass more, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea it will but in the interest of not having to write a specific conversion case for all these unexpected types I just defaulted to using fmt.Sprintf. The only reason this case is actually really in here instead of letting them all fallthrough to the default was to please the linter.

Copy link
Contributor

@enver-bisevac enver-bisevac left a comment

Choose a reason for hiding this comment

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

LGTM, minor comments on Sprintf

@jcox250 jcox250 merged commit 6bb5f78 into main Apr 5, 2022
@jcox250 jcox250 deleted the FFM-2934 branch April 5, 2022 11:56
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.

None yet

2 participants