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

feat: support parsing of complex options #1744

Merged
merged 28 commits into from
Jul 7, 2022

Conversation

NikhilVerma
Copy link
Contributor

This allows parsing of complex options and fixes #1743

alexander-fenster and others added 22 commits April 8, 2021 20:47
* test: adding test for pbjs static code generation

* fix: fromObject should not initialize oneof members
This option skips generation of service clients.

Co-authored-by: Alexander Fenster <fenster@google.com>
* deps: set @types/node to star version

When using `protobuf.js` as a dependency in a project it is important
that `@types/node` package gets de-duped and has the same version as for
the rest of the modules in the project. Otherwise, typing conflicts
could happen as they do between v13 and v14 node types.

* fix: use @types/node >=13.7.0

* fix: use @types/node >=13.7.0

Co-authored-by: Alexander Fenster <fenster@google.com>
Co-authored-by: Alexander Fenster <github@fenster.name>
Co-authored-by: xiaoweili <xiaoweili@tencent.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@NikhilVerma
Copy link
Contributor Author

@alexander-fenster Will be glad if you could take a look at this

@emarkk
Copy link

emarkk commented Jun 16, 2022

+1

@NikhilVerma
Copy link
Contributor Author

In the meanwhile this fix is published as https://www.npmjs.com/package/@cldcvr/protobufjs because I needed it to make https://www.npmjs.com/package/@cldcvr/protobufjs-typescript-gen

@alexander-fenster alexander-fenster changed the title Support parsing of complex options feat: support parsing of complex options Jun 17, 2022
Copy link
Contributor

@alexander-fenster alexander-fenster left a comment

Choose a reason for hiding this comment

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

Thank you! This will go into v7.0 that we are going to publish next week.

@alexander-fenster
Copy link
Contributor

@NikhilVerma Would you please fix linting, or grant the contributors write access to your branch? Thank you!

@NikhilVerma
Copy link
Contributor Author

@alexander-fenster Thank you! I've added you as the collaborator to my repo

@NikhilVerma
Copy link
Contributor Author

@alexander-fenster I've fixed the lint issues thanks!

@alexander-fenster alexander-fenster changed the base branch from 6.x to master July 7, 2022 01:59
@alexander-fenster
Copy link
Contributor

Hi @NikhilVerma,

I switched your PR to be against master so that it goes into 7.0 that we are preparing. Do you mind taking one more look at the diff just to make sure I didn't screw up things? (I needed to resolve a bunch of conflicts)

Also, the CI should be green now so we are good to go after some extra 👀 .

Thank you!

@alexander-fenster
Copy link
Contributor

Actually, the tests now fail with

not ok 848 should take last repeated int in last repeated option

do you have an idea why is that? I can also take a look but it will take me longer to figure out.

@NikhilVerma
Copy link
Contributor Author

@alexander-fenster While checking I realised I had written some unnecessary code, it already handled arrays just didn't handle them in all conditions. So I've simplified the code and the tests pass now with the simpler code.

@alexander-fenster alexander-fenster merged commit b1746a8 into protobufjs:master Jul 7, 2022
@NikhilVerma
Copy link
Contributor Author

@alexander-fenster Amazing! thanks for the collaboration :) Looking forward to v7

This was referenced Jul 8, 2022
@emarkk
Copy link

emarkk commented Jul 11, 2022

I'm not sure the issue was fixed though. I have the following Protobuf:

message AwesomeMessage {
  option (option.extension) = {
    scopes: [SCOPE_PRODUCT]
    visibility: VISIBILITY_PUBLIC
  };

  repeated Label labels = 1;

  LabelSource labels_source = 2 [(option.field).visibility=VISIBILITY_INTERNAL];
}

and decoding still fails with error illegal value '[' in line 17 (the line that has scopes: [SCOPE_PRODUCT]).
I thought this PR would fix the issue 😬

@NikhilVerma
Copy link
Contributor Author

@emarkk Are you using v7.0.0? I just tried this on v7 and it worked fine

syntax = "proto3";

message AwesomeMessage {
  option (option.extension) = {
    scopes: [SCOPE_PRODUCT]
    visibility: VISIBILITY_PUBLIC
  };

  repeated Label labels = 1;

  LabelSource labels_source = 2 [(option.field).visibility=VISIBILITY_INTERNAL];
}

enum LabelSource {
  LABEL_SOURCE_UNKNOWN = 0;
  LABEL_SOURCE_USER = 1;
  LABEL_SOURCE_AUTO = 2;
}

enum Label {
  LABEL_UNKNOWN = 0;
  LABEL_USER = 1;
  LABEL_AUTO = 2;
}

NikhilVerma added a commit to nonfx/protobufjs-typescript-gen that referenced this pull request Jul 11, 2022
@emarkk
Copy link

emarkk commented Jul 11, 2022

Ok, my bad. It was just a cache issue. Works fine with v7. Thanks a lot!

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.

Option value parsing is only supported partially
8 participants