Comments (5)
@nilslice I think I may have confused you. Here's an example of what I have.
service TestService {
rpc getTestObjects(TestRequest) returns (TestResponse)
// @protolock:skip
rpc getTestObjectsUnstable(UnstableRequest) returns (UnstableResponse) {
option (incubating) = "true";
}
}
So my intention is to make sure that any "incubating" RPC's are left out of the proto.lock file. So my custom rule would basically say that if it finds any RPC's with the "incubating" option throw a warning suggesting to add the "@protolock:skip" comment.
However more and more as I think about this, this doesn't sound like a version compatibility rule as much as a linter type rule. So while adding the ability for RPC's to have comments and skip logic may be valid, I am going to back off from this feature suggestion as my use case doesn't feel valid. Feel free to either keep this issue open for future development or close.
Thanks again for your input.
from protolock.
Maybe a configuration for the parser including comments that would default to false?
That is something we could certainly consider... If you have some time to take a stab at this, feel free. If you find a solution that meets your needs this way, we can merge it.
If not, I'm mainly concerned with the possibility of bloating file size, and needing to deserialize excess data in the form of comments. I understand your use case completely, but have to reject the inclusion of comments (at least for now, in lieu of what might be a ton of testing to see how comments impact parsing speed, file size, etc).
Alternatively, I would suggest adding another option
to the field, where required=true
or similar. This is probably the easiest way to get protolock support for your check (assuming I understand the goal).
There may be other ways to consider. Would you be able to share an example of a message with fields that you'd like to check for? And if possible, include the warning you'd like to issue when/if the plugin finds a problematic field?
from protolock.
@evanlknapp - I see, though I'm not sure about storing literal comments in the proto.lock
file.. does adding a no-op option
to the RPC work instead? You don't need to use the option in your codebase, but rather to decorate and check the RPC for catching in your plugin.
I would love to have support for // @protolock:skip
on the RPCs (and every other data type in addition to message). If that is a preferable method for you, I would recommend we go this route:
- add the skip logic to RPC types
- add a
Skipped
boolean to each type that can interpret a skip hint rather than omitting the type from theproto.lock
file. - modify the logic to skip applying ruleFuncs to data types if
x.Skipped == true
Though there could be a big difference between an "incubated" RPC being addressed with a warning, and a generally "skipped" RPC which you simply want protolock to ignore.
I'd gladly accept a PR that implements the above (or something similar if you have other ideas) -- I personally will have limited time to look into this for a couple weeks. Let me know what you think, thanks!
from protolock.
Hey @nilslice so I am circling back to this one to revisit the inclusion of comments. Generally I would agree with you about being skeptical about including comments in the proto.lock
file, however my workplace has a use case. So now that required
as a keyword has been removed in proto3
we have taken to indicating that a field for a message is required (mainly for our docs, as we enforce it manually in our implementation) by having a field comment including the string (Required)
.
I was hoping to implement a rule for my custom plugin that would prevent adding required fields to a message. To do this I would need to have comments accessible in the proto.lock
file. While I do agree with your feelings on comments I was wondering if you could think of any way I could accomplish this without changing how our protobuf files indicate required. Maybe a configuration for the parser including comments that would default to false? Suggestions are welcome.
from protolock.
As an example:
Stable version:
message PersonMessage {
// (Required) Person's First Name
string firstName = 1;
string lastName = 2;
}
Updated version:
message PersonMessage {
// (Required) Person's First Name
string firstName = 1;
// (Required) Person's Last Name
string lastName = 2;
}
I would like the updated version to show and error as it has added a required field saying something like: Message "PersonMessage" added field "lastName" as a required field.
from protolock.
Related Issues (20)
- Cannot parse certain Protobuf files HOT 5
- Deterministic definition order. HOT 4
- What are the differences to Uberβs Prototool HOT 1
- use yaml for proto.lock to reduce diff HOT 6
- Plugin wiki readme json missing fields HOT 2
- Lock file does not record aggregate options with array values HOT 3
- Website incorrectly reports backwards compatibility with invalid proto files HOT 2
- Error building protolock on ppc64le HOT 2
- Processing a single file HOT 3
- Enum aliases not supported HOT 3
- Does protolock support proto2 ? HOT 1
- Propose to publish ARM64 binary in releases HOT 6
- Convert circleci jobs to GitHub Actions
- oneof structure is not persisted in protolock HOT 4
- Options not parsed at the Enum level HOT 2
- found "stream" but expected [rpc method] for a valid proto HOT 3
- Add artefacts to Maven repository HOT 5
- Update docker hub image HOT 3
- Allow ability to include proto files that are not in the root tree
- TestOrder consistently fails
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. πππ
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google β€οΈ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from protolock.