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

Investigate allowing optional package parameters when parsing Protobuf schema #318

Closed
dmarticus opened this issue Aug 12, 2020 · 7 comments · Fixed by #324
Closed

Investigate allowing optional package parameters when parsing Protobuf schema #318

dmarticus opened this issue Aug 12, 2020 · 7 comments · Fixed by #324
Labels
enhancement New feature or request

Comments

@dmarticus
Copy link
Contributor

dmarticus commented Aug 12, 2020

Per this discussion in gitter

Anton Semenov @a7emenov 04:40
@dmarticus thank you for the response, really appreciate it! Basically confirms all my assumptions. Though I really love Mu, this may be the reason why we'd have to switch to ScalaPB as it allows to be very flexible when generating packages.
Is it likely that in the future skeuomorph and, by extension, mu will support options like java_package?

It seems like at least one person would want us to allow for optional package generation within skeuomorph and mu. I'm not sure at this point how significant of an undertaking that work would be but I figured it's worth making a task to track it so we can triage at some point in the future

@dmarticus dmarticus added the enhancement New feature or request label Aug 14, 2020
@a7emenov
Copy link

@dmarticus, to add some clarity, my problems with the package option are:

  • It is mandatory for skeuomorph to function, but not for protoc.
  • It influences where the resulting files will be generated. While there are workarounds around this, I am inclined to believe it should have only "semantic" meaning, i.e. how services should be referenced.

As for a real-world example, my original .proto files do not have the package option and I cannot really change them as they are used across multiple teams. But even adding the package option is kind of fruitless without the muSrcGenIdiomaticEndpoints as clients generated by protoc will not be able to reference the service without the package prefix.

@dmarticus
Copy link
Contributor Author

dmarticus commented Aug 25, 2020

@a7emenov do you have an example protobuf file that I could work with that you'd like to be able to use with mu and skeuomorph? I'm planning on implementing this behavior this week and I'd love to have a file that you want to be able to convert to use as a test case

@a7emenov
Copy link

a7emenov commented Aug 25, 2020

@dmarticus, sure, thank you so much for a swift response.

Example 1:

  • File:

    syntax = "proto3";
    option java_package = "my_package";
    
    service MyService {
      rpc Check(MyRequest) returns (MyResponse);
    }
    
    message MyRequest {
      string value = 1;
    }
    
    message MyResponse {
      string value = 1;
    }
    
  • Expected:
    MyService service will be generated under "my_package" package.

  • Actual:
    Service generation fails as the package option is not provided.

Example 2:

  • File:

    syntax = "proto3";
    package other_package;
    
    option java_package = "my_package";
    
    service MyService {
      rpc Check(MyRequest) returns (MyResponse);
    }
    
    message MyRequest {
      string value = 1;
    }
    
    message MyResponse {
      string value = 1;
    }
    
  • Expected:
    MyService service will be generated under "my_package" package.

  • Actual:
    MyService service is generated under "other_package" package.

@dmarticus
Copy link
Contributor Author

Thank you for sharing! These will be some great test cases for my implementation; I appreciate it!

@dmarticus
Copy link
Contributor Author

@a7emenov we should be releasing this change this week! Thanks for your help in providing the context around it and I hope you find the changes helpful :)

@a7emenov
Copy link

@dmarticus, I can't properly express how much this helps! Much obliged for acting so swiftly, this basically allows to keep using mu in the project. Looking forward to the release!

@dmarticus
Copy link
Contributor Author

@a7emenov my pleasure! It's great to have users who push the envelope of what we've made and identify gaps, so I appreciate you for that. The release went out today (https://gitter.im/47deg/mu?at=5f4d6fa5dfaaed4ef521757a), and I'll publish the changes to sbt-mu-srcgen shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants