TheThingsIndustries / protoc-gen-fieldmask

Generate field mask utilities from proto files
Apache License 2.0
10 stars 3 forks source link

Modularize and add validation #22

Closed rvolosatovs closed 5 years ago

rvolosatovs commented 5 years ago

Summary:

Closes #21 Closes #18 References https://github.com/TheThingsNetwork/lorawan-stack/issues/51

Changes:

Notes for Reviewers:

This PR uses forks of both https://github.com/lyft/protoc-gen-star (it uses features in https://github.com/lyft/protoc-gen-star/pull/51) and https://github.com/lyft/protoc-gen-validate(minor changes there to improve GoGo support and use fieldpaths)

I forked protoc-gen-validate, since only minor changes are necessary to make it work with fieldpaths(https://github.com/TheThingsIndustries/protoc-gen-validate/commit/0e040cde90b8bd2699b43cfb67d24049f11a1e54), however we do not need to maintain any validators and we're fully compatible with the upstream plugin.

cc @KrishnaIyer

rvolosatovs commented 5 years ago

Here's how the generated code looks like for lorawan-stack: https://github.com/rvolosatovs/lorawan-stack/commit/9d36c19dda21e82c01a32c2626285f2fcb93f4c2 It's a bit rough around the edges, but good enough for MVP Note, that there still is a little issue with CustomName handling in oneofs, which I will have to fix in https://github.com/TheThingsIndustries/protoc-gen-star before this can be merged. I will add a testcase for that too.

rvolosatovs commented 5 years ago
  1. Fixed that in https://github.com/TheThingsIndustries/protoc-gen-validate
  2. We do get the full path when error occurs: invalid Test.a: embedded message failed validation | caused by: invalid Test_TestNested.a: embedded message failed validation | caused by: invalid Test_TestNested_TestNestedNested.a: value must be inside range (24, 42] We may want to use the proto names for types here also, but that's not a priority now - please create an issue at https://github.com/TheThingsIndustries/protoc-gen-validate if this is required