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 #644 #651

Closed
wants to merge 1 commit into from
Closed

fix #644 #651

wants to merge 1 commit into from

Conversation

yahelmanor
Copy link

Fix by change the condition in the if statement from the problematic reflect based condition to safer one.
I try before this solution, other reflect based solution but something in the reflect package seams to not work like i was expected.

fix by change the condition in the if statement from the problematic reflect based condition to safer one.
@devdinu
Copy link
Contributor

devdinu commented Aug 22, 2018

@yahelmanor Could you share what wasn't working with reflect check, please share code or test which 's breaking and that would be assisting the need for change as well.

@yahelmanor
Copy link
Author

yahelmanor commented Aug 22, 2018

the test that panics (from the #644 issue) is

package main

import (
        "testing"

        "github.com/stretchr/testify/assert"
)

type MyType string

const (
        MyTypeA MyType = "a"
        MyTypeB MyType = "b"
)

func TestMyTypeRequire(t *testing.T) {
        mt := MyTypeA
        assert.Equal(t, mt, MyTypeA)
        assert.Equal(t, mt, MyTypeB)
}

i try to replace the original condition of ek != reflect.String that is checking that exacted variable have the same kind as string, this check not cover the case of kind is equal to string but you still can`t convert it.

a small demonstration to this problem is in this program
playground: https://play.golang.org/p/XR7aZZSBmSY

package main

import (
	"fmt"
	"reflect"
)

type s string

func main() {
	var sA s = "a"
	i := interface{}(sA)
	fmt.Println("sA,i kinds =", reflect.TypeOf(sA).Kind(), ",", reflect.TypeOf(i).Kind())
//print: sA,i kinds = string , string
	fmt.Println("can we convert to string by reflect?", reflect.TypeOf(i).ConvertibleTo(reflect.TypeOf("")))
//print: true
	fmt.Println("just to chack the type of <\"\"> is:", reflect.TypeOf(""))
//print: string
	_, ok := i.(string)
//here it panics in the test
	fmt.Println("can we actualy convert?", ok)
//print: false
	s, ok := reflect.ValueOf(i).Convert(reflect.TypeOf("")).Interface().(string)
	fmt.Println("and via reflect?", ok, ",s =", s)
//print: and via reflect? true ,s = a
}

in the demonstration the kind of sA is string but still it can`t be convert.

so it can be possible to change the assert statement into the reflect way but i change the condition so in case that make the original problem it will go throw the spewConfig.Sdump function that use reflect so i think there is no need to use the reflect conversion in that case but it open to discussion.

@georgelesica-wf
Copy link

I believe this issue has been addressed. Feel free to re-open and explain if that is not the case. Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants