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

Add Java options for a correct generation #295

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

disk91
Copy link

@disk91 disk91 commented Mar 1, 2023

These options helps to generate Java classes for Config service properly

Copy link
Member

@madninja madninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a java CI build? That way we'll all know when stuff breaks

Copy link
Member

@madninja madninja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment and I'd still like to see a Java CI step to ensure this does not break in future builds

@@ -1,6 +1,9 @@
syntax = "proto3";

package helium;
option java_package = "xyz.nova.grpc";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you rename this to com.helium.* please? This repo is in the foundation GitHub team

@disk91
Copy link
Author

disk91 commented Mar 20, 2023

Hello, It has been updated and the new files with header have been added

@@ -1,6 +1,9 @@
syntax = "proto3";

package helium;
option java_package = "com.helium.grpc";
option java_outer_classname = "regionparam";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regionparam and not RegionParam?

Copy link
Contributor

@jeffgrunewald jeffgrunewald Mar 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or IotRegionParam for consistency with the region.proto outer class name?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, the generation is not working properly when you name it correctly. I do not remind the detail of the problem but it was not working with camelcase and upfront uppercase

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how was the problem exhibited? At build time? If so, I would still like to see a CI step, just like we do for all other supported languages

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

Successfully merging this pull request may close these issues.

4 participants