letmegrpc icon indicating copy to clipboard operation
letmegrpc copied to clipboard

invalid code is generated: conn declared and not used

Open feliksik opened this issue 7 years ago • 4 comments

I am trying to run letmegrpc but the generated code is incorrect:

letmegrpc --addr=localhost:3149 --port=8080 listdates.proto 
2017/03/10 11:00:35 # tmpprotos
../../listdates.letmegrpc.go:54: conn declared and not used

Unfortunately there is a lot of implicit stuff going on. I found the generated file is in some /tmp/letmegrpc_ directory, and the code is indeed incorrect:

func NewHandler(grpcAddr string, stringer func(req, resp interface{}) ([]byte, error), opts ...google_golang_org_grpc.DialOption) (net_http.Handler, error) {
        conn, err := google_golang_org_grpc.Dial(grpcAddr, opts...)
        if err != nil {
                return nil, err
        }
        mux := net_http.NewServeMux()
        return mux, nil
}

Indeed this is not correct go code. It is generated with this code

What can we do about it?

Related question: I see the code generator uses gogo/protobuf code generator, with .In(), .Out(), etc. Is there a reason to do it like this? Would it not be much simpler to generate the code with a template, and format it with gofmt afterwards?

feliksik avatar Mar 10 '17 10:03 feliksik

The code is assuming you have defined some services. Maybe the error could be better, but I think thats a fair assumption or not?

Also this letmegrpc --addr=localhost:3149 --port=8080 listdates.proto is the quickstart. If you want to be more professional later please use protoc -letmegrpc_out=. grpc.proto

I prefer getting compile errors vs runtime errors for my code generation when it is so complex. That is why I prefer not to use templates in the cases where I am generating a large amount of code. But maybe this case could have been done with templates, hmmm.

awalterschulze avatar Mar 10 '17 10:03 awalterschulze

The code is assuming you have defined some services. Maybe the error could be better, but I think thats a fair assumption or not?

I think it's a fair requirement that there are some services given, and I actually didn't defined it properly. But it's not a valid assumption IMHO, in the sense that this it should generate an error as an invalid input was received. I think the generator should fail if file.GetService() returned no entries.

I prefer getting compile errors vs runtime errors for my code generation when it is so complex.

I totally agree, but I honestly don't see how this is facilitated by your generator, and impossible with templates. But I may be overlooking something here -- I haven't worked on such code generation yet.

feliksik avatar Mar 15 '17 14:03 feliksik

Would you like to contribute this error reporting?

When you are using templates same for example

Name: {{.Name}}

And you have a struct

type A struct {
  FirstName string
}

The go compiler is not going to tell you that your struct field should be called Name and not FirstName or that your template should be changed. This is what I mean with a runtime error. Granted you can put this on init and get the error really quickly.

If someone wants to take over the maintenance of this project. I won't stand in the way of this design choice.

awalterschulze avatar Mar 15 '17 14:03 awalterschulze

I created a PR to do the error reporting, but don't have time to test it now.

I think it solves the right problem and the interface is pretty, but I haven't been able to use it properly so far. I hope to be able to study letmegrpc and your comment more in-depth later! Thanks Walter.

feliksik avatar Mar 15 '17 15:03 feliksik