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

Fix generating nested enums in proto files with no package #28

Merged
merged 2 commits into from
Sep 19, 2018
Merged

Fix generating nested enums in proto files with no package #28

merged 2 commits into from
Sep 19, 2018

Conversation

bbqsrc
Copy link
Contributor

@bbqsrc bbqsrc commented Sep 17, 2018

if you have a proto that looks like the following:

syntax = "proto3";

message Foo {
  enum Bar {
    A = 1;
    B = 2;
  }
  Bar bar = 1;
}

It will fail to build. This is because the enumMap is adding Foo.Bar as .Foo.Bar. This patch fixes that.

@agreatfool
Copy link
Owner

Hello,

Thank u for ur PR.

I just test your issue with actions below, and the building succeeded without any error. Could you please show me your versions:

  • node --version
  • npm --version
  • protoc --version
  • grpc_tools_node_protoc --version
  • npm list -g --depth=0 | grep grpc-tools

My test steps:

  • Edit code examples/proto/book.proto, add your test message described in your description
  • Run bash examples/bash/build.sh
  • See result

As my result, new definition generated like below:

export class Foo extends jspb.Message { 
    getBar(): Foo.Bar;
    setBar(value: Foo.Bar): void;


    serializeBinary(): Uint8Array;
    toObject(includeInstance?: boolean): Foo.AsObject;
    static toObject(includeInstance: boolean, msg: Foo): Foo.AsObject;
    static extensions: {[key: number]: jspb.ExtensionFieldInfo<jspb.Message>};
    static extensionsBinary: {[key: number]: jspb.ExtensionFieldBinaryInfo<jspb.Message>};
    static serializeBinaryToWriter(message: Foo, writer: jspb.BinaryWriter): void;
    static deserializeBinary(bytes: Uint8Array): Foo;
    static deserializeBinaryFromReader(message: Foo, reader: jspb.BinaryReader): Foo;
}

export namespace Foo {
    export type AsObject = {
        bar: Foo.Bar,
    }

    export enum Bar {
    A = 0,
    B = 1,
    }

}

@bbqsrc
Copy link
Contributor Author

bbqsrc commented Sep 18, 2018

The commit is small enough that you can see it is a simple logic error. If the scope is a blank string, it will be prefixed with a . regardless because the variable that was created explicitly to work around that case was not reused in the correct number of positions.

$     node --version
v10.8.0
$     npm --version
6.4.0
$     protoc --version
libprotoc 3.6.0
$     grpc_tools_node_protoc --version
libprotoc 3.4.0
$     npm list -g --depth=0 | grep grpc-tools
├── grpc-tools@1.6.6```

@agreatfool
Copy link
Owner

I just tested again, and got the idea.

The reason why simple test code you provided failed is that code has no package defined. And previously I just appended the test code in example code, and it has package already defined, so no error encountered.

Critical issue code is here:

const scope = fileDescriptor.getPackage();

I will merge it asap.

@agreatfool agreatfool merged commit a272dae into agreatfool:master Sep 19, 2018
@agreatfool
Copy link
Owner

Just merged & new version 2.3.2 has been published. Thank you for your PR.

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.

2 participants