Skip to content
This repository has been archived by the owner on Jun 15, 2020. It is now read-only.

Add support for proto3 maps #8

Merged
merged 17 commits into from
Apr 27, 2016

Conversation

DouglasHeriot
Copy link
Contributor

Implements #6 generating maps.
Only tested a little, but works for me.
Have only implemented support for using properties, not the get/set methods (I’ve never used get/set on a map). Should be straightforward if someone else wants to do so.

I have not tested yet with maps where keys are types other than string – have to see what Protobuf.JS generates in that case.

michelgb and others added 11 commits February 6, 2016 10:46
* Added dustjs-helpers package, so you can use @eq for basic logic in templates. It's a weird wrapper around dustjs-linkedin, so you require() it instead.
* Added tsconfig.json file, so you can simply run "tsc" to compile everything.
It's a little confusing - I think dependencies is for things users of the library need. devDependencies are required for building and testing the library. http://stackoverflow.com/a/22004559/466698
Better support for proto3 map methods, and ProtoBuf.Js Long type.
@DouglasHeriot
Copy link
Contributor Author

Improved support for maps, especially with different key types. Merged in michelgb’s branch.

Not sure why, but in 8686a72 it had "Message" appended to some generated message types? The functions that were previously in a superclass are now generated in a new derived class? I don’t know if that is closer to what ProtoBuf.js does maybe?

Anyway, it would be good to merge this pull request, with much better support for maps.

DouglasHeriot and others added 6 commits February 14, 2016 15:13
I get why it was done that way now - allow type-checking of objects passed to constructor, without requiring them to be message objects with encode functions and other stuff.
…e names.

Useful when using ProtoBuf.convertFieldsToCamelCase to match the JSON format expected by officialy proto3 JSON parsers (eg. in C++)
case 'sint64':
case 'fixed64':
case 'sfixed64':
return "Long";
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like the tests were run for this change - this results in a Cannot find name 'Long'. (TS2304) error since "Long" is not a thing in typescript?

@DouglasHeriot
Copy link
Contributor Author

I did run the tests, and they all pass. Long is a type provided by Protobufjs typings – see long.d.ts in the definitions directory.

@fungiboletus fungiboletus merged commit 20503d8 into SINTEF-9012:master Apr 27, 2016
@fungiboletus
Copy link
Member

Hei,

The tests pass on my computer as well (clean install).
I merged the branch and created a new tag (2.0.0-beta). Sorry about the long delay to merge your pull-request, I had your code on my computer but I forgot to merge it…

@fungiboletus
Copy link
Member

Thank you for the contribution :-)

@DouglasHeriot
Copy link
Contributor Author

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants