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

invalid code is generated: conn declared and not used #34

Open
feliksik opened this issue Mar 10, 2017 · 4 comments
Open

invalid code is generated: conn declared and not used #34

feliksik opened this issue Mar 10, 2017 · 4 comments

Comments

@feliksik
Copy link

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?

@awalterschulze
Copy link
Member

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.

@feliksik
Copy link
Author

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.

@awalterschulze
Copy link
Member

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.

@feliksik
Copy link
Author

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.

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

No branches or pull requests

2 participants