search

fullstorydev / grpchan

Channels for gRPC: custom transports
MIT License
204 stars 23 forks source link

remove default case on parse arg #35

Closed fzerorubigd closed 5 years ago

fzerorubigd commented 5 years ago

After updating this to the new version, there is an issue using the prototool : 

<input>:1:1:protoc-gen-grpchan: grpchan: unknown plugin argument: Mpath/to/proto/file.proto

the issue is the Mpath is required for the plugin, but this proto blocks it.

jhump commented 5 years ago

the issue is the Mpath is required for the plugin

I don't follow this. The M argument is not required for any plugin, and I don't know of any plugin that supports it other than protoc-gen-go. At first glance, this sounds like a bug against prototool -- it should only be supplying it to the protoc-gen-go plugin (e.g. in the --gen_out parameter to protoc).

However, if that argument is needed to generate the correct Go code, then that may instead be an enhancement request for grpchan to actually support that argument. But in that case, this PR is still inappropriate: it should actually support these parameters (and use them when computing the Go path and package for a proto file), not ignore them.

jhump commented 5 years ago

FYI, I am looking into just adding proper support for both M... and the import_path arguments that protoc-gen-go supports.

fzerorubigd commented 5 years ago

Fine by me. but the problem is (inappropriate or not) this is somehow breaking BC on the minor version, (from 1.0.0 to 1.0.1) I fixed it by using go mod replace for now, and looking forward to your proper solution. BTW for reproducing this you can use prototool and add the plugin with 

    - name: grpchan
      type: gogo
      output: .
jhump commented 5 years ago

@fzerorubigd, I think the issue might be that you have type: gogo in your config, but this is not a gogo protoc plugin. So it's trying to pass the parameters that protoc-gen-gogo expects.

Though with #36, you could keep that (or use type: go) and the grpchan plugin will actually use the provided options when generating code.

fzerorubigd commented 5 years ago

actually, it was type: go, at first, I didn't realize the update, I tried this to make sure if the problem is related to this or not. the type: go has the same effect.

jhump commented 5 years ago

It needs to have no type. Both gogo and go accept those parameters, which is why it is passing them to the grpchan plugin incorrectly. Just remove the type.

When I mentioned type: go above, it was that you could use it after PR #36 lands. It's now landed, but I think I've found an issue with it, so I have not cut a new release yet. But if you want to point your stuff at master of this repo, you could try again with type: go.

fzerorubigd commented 5 years ago

Thank you, to be honest, I didn't know about no type. I can confirm it works without type.