-
Notifications
You must be signed in to change notification settings - Fork 74
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
add csharp_namespace option #227
Comments
@bestbeforetoday Can I create a pr? |
@Varorbc historically the fabric protos have included language specific options like go_package, which isn't ideal. The language bindings now get generated using Buf, which has a managed mode to take care of those options outside the proto files, which works really well. Buf managed mode seems to have C# support -> https://buf.build/docs/generate/managed-mode#c# (It would be really interesting to get a PR to add C# to the three current language bindings that get generated.) |
@jt-nti Thank you for providing another solution. But for users who use C#, the function of generating code according to proto has been provided, and we may prefer to choose this way instead of introducing external dependencies.It would be great if you could add the namespace option to the proto file. |
@Varorbc I wouldn't object to csharp_namespace options being added to the proto files but experience with the Go, Java and Node bindings is that it is much easier for API consumers to just use a pre-generated module instead of having to generate anything themselves. I don't know C# but I assume that it would be possible to do the same kind of thing, in which case building on the existing Buf framework in this repo to generate and publish C# bindings would be ideal. |
I've been experimenting with adding C# bindings in #230 but I'm not really sure what's required to make it a useful/publishable package. Any ideas welcome! |
I have done message binding and implemented the gateway client with reference to Java. After you release the preview package, I will replace it. |
@Varorbc I notice you closed your two pull requests to add csharp namespace options to the protobuf files. Did you find a different approach to make things work for your csharp client API implementation? It would be great to share your implementation with the community. If it's complete enough for others to use, it might even be something you want to contribute as a new Hyperledger lab or project. |
@bestbeforetoday Sorry, but this has been dragging on for too long, so I temporarily copied protobuf into my project and added the C# namespace options. Right now, our client is working well with Hyperledger Fabric. As for the experimental project you mentioned, I need to get approval from my boss since I don’t have the authority to decide on this. |
Generating the protobuf bindings within your own project is definitely a viable approach. This used to happen with most of the official Fabric client SDKs too. It's only when we released the Fabric Gateway client API alongside Fabric v2.4 that we created the current artifacts for Node and Java, and the Go bindings based on the Go protobuf APIv2. |
@bestbeforetoday generating protobuf bindings yourself is definitely viable but it would be nice to at least get the namespaces in the official protobuf definitions. I think @Varorbc was pretty close to having that done (I would have liked to see a minor change #228 (comment) but that's not a huge issue) unfortunately none of the project maintainers reviewed the PRs. I've not had time to do any more investigation on publishing pre-generated bindings but I would be happy to create new PRs to get the namespaces in if anyone is willing to approve them. |
@bestbeforetoday If multiple projects need protobuf, then generating a unified binding for protobuf makes sense. But if it’s just one project, setting it up directly in that project should be fine. Also, I talked to my boss about contributing to the Fabric Gateway C# client API as a Hyperledger experimental project, and he said he’ll consider it. |
No description provided.
The text was updated successfully, but these errors were encountered: