smnbbrv / ngx-grpc

Angular gRPC framework
MIT License
239 stars 34 forks source link

Package name with uppercase letters is not properly generated #87

Open esoubiran-aneo opened 2 years ago

esoubiran-aneo commented 2 years ago

Let's assume the following proto file:

syntax = "proto3";

package ArmoniK.api.grpc.v1.submitter;

service Submitter{
    rpc CancelSession (Session) returns (Empty);
}

message Session {
    string id = 1;
}

message Empty {}

The generator will not work because it wil generate this:

Observable<GrpcEvent<thisProto.ArmoniK.api.grpc.v1.submitter.Empty>>

instead of this

Observable<GrpcEvent<thisProto.Empty>>
smnbbrv commented 2 years ago

hi @esoubiran-aneo

could you please share the version of protoc?

esoubiran-aneo commented 2 years ago

I think it's in this package ? @smnbbrv

"grpc-tools": "^1.11.2",
"@ngx-grpc/protoc-gen-ng": "^3.0.1"
esoubiran-aneo commented 2 years ago

But I use Windows ! I will try the linux command to see if there is a difference

esoubiran-aneo commented 2 years ago

Yes, this is using protoc 3.6.1 on Ubuntu (libprotoc 3.6.1)

cancelSession: (
    requestData: thisProto.ArmoniK.api.grpc.v1.Session,
    requestMetadata = new GrpcMetadata()
  ): Observable<GrpcEvent<thisProto.ArmoniK.api.grpc.v1.Empty>> => {
    return this.handler.handle({
      type: GrpcCallType.unary,
      client: this.client,
      path: '/ArmoniK.api.grpc.v1.Submitter/CancelSession',
      requestData,
      requestMetadata,
      requestClass: thisProto.ArmoniK.api.grpc.v1.Session,
      responseClass: thisProto.ArmoniK.api.grpc.v1.Empty
    });
  }

(ArmoniK.api.grpc.v1 is not defined)

with this package: package ArmoniK.api.grpc.v1;

smnbbrv commented 2 years ago

I guess this is an issue and it should be fixed on a lib side. However, according to the google protobuf styleguide

Package names should be in lowercase. Package names should have unique names based on the project name, and possibly based on the path of the file containing the protocol buffer type definitions.

Please use the lowercase names only for packages and the issue won't appear.

esoubiran-aneo commented 2 years ago

You're a king man! It seems to work now that package name is "armonik.api.grpc.v1"!

smnbbrv commented 2 years ago

you are welcome! I'm glad it worked out!

Please leave the issue open, when there is time to deal with it, it's gonna be fixed