Arshia001 / FSharp.GrpcCodeGenerator

A protoc plugin to enable generation of F# code + supporting libraries
MIT License
81 stars 9 forks source link

Various fixes in code generation #8

Closed purkhusid closed 2 years ago

purkhusid commented 3 years ago

What?

Code Generation fixes

Added a test suite

Updated Protobuf.FSharp

This was required because we needed to regenerate the Descriptor.fs for Proto3 optional support

purkhusid commented 3 years ago

@Arshia001 This PR got a bit bigger since I decided to fix various issues with the code generation. Also working on adding some tests.

Arshia001 commented 3 years ago

Wow, good job! Reviewing all of this will take some time though. Are you done yet, or are there more changes coming?


Edit: I just noticed your todo list above. I'll stress this point again: the code generator was not designed to interoperate with C# and I still don't think it's a good idea.

purkhusid commented 3 years ago

@Arshia001 The interop comment is not about using the C# libraries in the generated code. I'm going to add some basic tests where I serialize/deserialize some protos with both the F# and the C# code to make sure that we are not messing up the serialization/deserialization.

Arshia001 commented 3 years ago

That's a great idea.

purkhusid commented 3 years ago

@Arshia001 This is ready for review. I have not added any conformance tests but I added a test for #6

brian-scb commented 3 years ago

Thanks for tackling, looks like it was a lot of work.

Should there be a new package for FSharp.Api.CommonProtos to parallel the C# version (Google.Api.CommonProtos) to hold the extensions? Then they would not have to be supplied separately for the tests.

purkhusid commented 3 years ago

@brian-scb It could be published as an separate package, but I'll let @Arshia001 decide. If we decide to do that we should add that in another PR

rogeralsing commented 3 years ago

Will this PR also take care of using C# generated protos as imports in F# generated protos? e.g. in Proto.Actor, we have the PID message which is a special Proto.Actor message. For us to be able to use this, it would have to be possible to import that PID type in the protos and then have the generated F# code reference that C# protobuf code.

Is that part of the plan/roadmap?

Arshia001 commented 3 years ago

@rogeralsing No, interop between the code generators is not a supported scenario; unless you have a reasonable suggestion on how it can be done.

purkhusid commented 3 years ago

Will this PR also take care of using C# generated protos as imports in F# generated protos? e.g. in Proto.Actor, we have the PID message which is a special Proto.Actor message. For us to be able to use this, it would have to be possible to import that PID type in the protos and then have the generated F# code reference that C# protobuf code.

Is that part of the plan/roadmap?

@rogeralsing Can you clarify how you do this in your current setup? Imports are generally declared in the .proto files and then you would generate F# code for all the files that are referenced.

E.g.

pid.proto

message Pid {
    int Id = 1;
} 

other.proto

import pid.proto

message UsingPid {
    Pid pid = 1;
}

You would then just run the C#/F# code generator for both the pid.proto and the other.proto and they should be wire compatible with each other.

rogeralsing commented 3 years ago

If we take this example: https://github.com/AsynkronIT/protoactor-dotnet/blob/dev/examples/Chat/Messages/Chat.proto#L4

Here we reference some core proto.actor messages in a custom user proto file. The generated C# output, will use the messages defined in the core proto.actor nuget. not regenerate the referenced protos.

Skärmavbild 2021-07-22 kl  11 28 34

The PID type there is the real PID type from Proto.Actor itself.

I believe it simply checks the csharp namespace of the imported messages and use that in the code genreation

purkhusid commented 3 years ago

Currently the only way to support this would be to have separate package for Proto.Actor.Protos.CSharp and Proto.Actor.Protos.FSharp and the users of Proto.Actor would have to reference either depending on if they were using F# or C#

Arshia001 commented 3 years ago

@rogeralsing see the discussion here: https://github.com/Arshia001/FSharp.GrpcCodeGenerator/issues/6#issuecomment-870876195

rogeralsing commented 3 years ago

Ah ok, then it's a no-go for us as the PID (and other) types are part of the core of Proto.Actor itself, and cannot be re-implemented elsewhere.

In our case, we would have needed something that generates the same kind of output as the C# generator, but with F# specific types. while still using the same type of referencing strategies.

Arshia001 commented 3 years ago

@rogeralsing since you're already using C# generated code in the base library, you can always use the C# version of the code generator. You can create a C# library and reference that from your F# code.

jberzy commented 3 years ago

Hi @Arshia001 and @purkhusid,

Any timeline for when this might get reviewed/merged? Or perhaps a test nuget repository? Reason I ask is because I'd like to try to get this to work with proto-actor.

Thank you!

Arshia001 commented 2 years ago

@purkhusid is it safe to assume the changes in this PR were also in #12, and have been merged in #16?

purkhusid commented 2 years ago

Yup, I'll close this one. Thanks for taking you time to review the changes!