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

Package information is missing in the [imported] messages. #129

Open
v1nce opened this issue Jun 4, 2021 · 1 comment
Open

Package information is missing in the [imported] messages. #129

v1nce opened this issue Jun 4, 2021 · 1 comment
Labels

Comments

@v1nce
Copy link

v1nce commented Jun 4, 2021

Hi,

First, thank you for this usefull tool.
I run pbf file.proto --browser > file.proto.js on simple file and it worked flawlessly.

Then I go for something more complex with no success.
I downloaded https://github.com/psobot/keynote-parser/protos and ran
pbf KNArchives.proto --browser > KNArchives.proto.js

It throwed "Unexpected type". I looked at the faulty js (compile.js) and go for a dirty fix
return prefix + '("'+field.type+'").read' + suffix; instead of throw new Error('Unexpected type: ' + field.type);
in the compileFieldRead and compileFieldWrite.

Then it compiled again but the output (500k lines) was not what I expected.
First I was happy because it means the tool supports "import" directive. I think it will not output 500k LOC for a 700 LOC input.
So it looked like I'll only have to run the command on top level *.proto file and not on every *.proto. Great.

Then I have a look at the detail of output. It looked like it does not support the package xxx; directive. (see KNArchives.proto line 10 for such directive)
This seems strange to me, because it supports the import directive.
Supporting one without supporting the other sounds odd to me because package seems easier to implement than import (I may be wrong)
The second things that puzzles me is the quickfix I had to write. Supporting "import" without supporting FieldType is odd.

So for those 2 reasons I think I should be doing something wrong.
**Could you clarify the following points ?

  • does pbf supports "FieldType references" other than the basic ones ? (I don't know how to call it, maybe Message or Struct would be more exact than "FieldType references")
  • does it support package directive ?
  • does it resolve import directive (whatever the nested level) ? So how does it deal with duplicate name functions (but in other packages)**
@v1nce
Copy link
Author

v1nce commented Jun 10, 2021

I dug a little deeper so here are my temporary conclusions :

Yes there is support for PACKAGE.
You fill find it in the json structure that is passed to the compile function

var proto = {
    "syntax": 2,
    "package": "TSK",
    "imports": ["TSPMessages.proto"],
    "enums": [...],
    "messages": [......],
    "options": {},
    "extends": []
}

But the support is incomplete
Here we see that the TSK package imports definitions from TSPMessages.proto.
So where are the messages from the TSPMessages ?
There are in the proto.messages.
How can you tell apart messages from package TSK and those from package TSP (the underlying package in TSPMessages) ?
You can't. The package information is lost. It's nowhere in a message.

Here is the JSON of a message
"messages": [{
"name": "TreeNode",
"options": {},
"enums": [],
"extends": [],
"messages": [],
"fields": [],
"extensions": null
}

See ? No Package information.

@v1nce v1nce changed the title no support for package in proto file ? Package information is missing in the [imported] messages. Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants