petergtz / pegomock

Pegomock is a powerful, yet simple mocking framework for the Go programming language
Apache License 2.0
254 stars 28 forks source link

Fix invalid chars in matcher file and function names #54

Closed danilvpetrov closed 6 years ago

danilvpetrov commented 6 years ago

Fixes genaration of names for matcher function and files. For instance, when a mock file and matchers are generated for an interface such as:

type Iface interface{
   DoSomeWork(s string, vars map[string]interface{}) error
}

Generated matcher file for map of strings to interface will result in name map_of_string_to_interface{}.go with the following content:

package matchers

import (
    "reflect"
    "github.com/petergtz/pegomock"

)

func AnyMapOfStringToInterface{}() map[string]interface{} {
    pegomock.RegisterMatcher(pegomock.NewAnyMatcher(reflect.TypeOf((*(map[string]interface{}))(nil)).Elem()))
    var nullValue map[string]interface{}
    return nullValue
}

func EqMapOfStringToInterface{}(value map[string]interface{}) map[string]interface{} {
    pegomock.RegisterMatcher(&pegomock.EqMatcher{Value: value})
    var nullValue map[string]interface{}
    return nullValue
} 

While a file with the name containing curly brackets is still usable, Go does not allow to have curly brackets in function names, which is currently has to be corrected manually every time the mock and matchers are regenerated.

petergtz commented 6 years ago

Hey @danilvpetrov,

This is great, thank you! Before I merge this, would you mind adding a test case that gets fixed by your change? I think that test case could be easily added to https://github.com/petergtz/pegomock/blob/master/mockgen/mockgen_test.go with a few lines.

Also, from your example it looks like this bug is very specific to interface{}, as it is the only interface type that has {} in the name, and which I haven't thought of. I was wondering if we should therefore simply check for interface{} instead of using a generic regexp. My concern is that by doing it through a regexp on all non-alphanumeric chars, we might miss potential bugs where the current code does not properly transform actual syntax elements. What do you think?

Again, thanks for your contribution!

danilvpetrov commented 6 years ago

Hi @petergtz, thanks for your prompt reply.

Sorry for not adding a unit test case test in the first place - fixed that.

I agree with adding just an interface check. I removed the previous regex bit.

Thanks for all your work on pegomock!

petergtz commented 6 years ago

Looks good! Merging. Thanks a lot for your contribution, @danilvpetrov.

danilvpetrov commented 6 years ago

Thank you very much, @petergtz!

petergtz commented 6 years ago

@danilvpetrov One more thing: The email you used for your commits does not match the email you're using for your github profile. For that reason, when you go e.g. to https://github.com/petergtz/pegomock/commit/65df2bd44412e269497d032cc9fa1b66b1a098af, it has your name in it, but it doesn't link to your account.

I think it would be a shame if you don't get proper credit for your commits, so you could do 2 things to fix this:

Of course it's totally up to you :-) Cheers, Peter