gRPC options #61
gRPC options #61
Conversation
| and PConstant = | ||
| | TIntLit of int32 | ||
| | TFloatLit of float | ||
| | TBoolLit of bool | ||
| | TStrLit of string | ||
| | TEnumLit of TIdent | ||
| | TMap of Map<string, string> |
There was a problem hiding this comment.
I understand the temptation to use a map here, but a map will destroy the order of the aggregated options (since it will sort the entries by key). Plus, the intention of this syntax is to provide shorthand for setting multiple fields on a single (option definition) message. E.g., the following "alternative aggregate syntax" sample from Protobuf Language Guide:
[(foo_options) = { opt1: 123 opt2: "baz" }]
is semantically equivalent to:
[(foo_options).opt1 = 123, (foo_options).opt2 = "baz"]
The first form is just shorthand for setting both opt1 and opt2 fields on the (foo_options) message. In fact, a future transform on the AST should probably flatten the first form into the second form, to simplify processing.
Therefore, IMHO a POption list would better represents the original source here: each option name (e.g., opt1) should be parsed to a TIdent (using pIdent_ws), and each value (e.g., 123) should be parsed to a PConstant (using pConstant_ws) rather than a string.
I would therefore suggest redefining TMap as the following and adjust the rest of your code accordingly:
| TAggregateOptionsLit of POption list
Note: Google's protoc.exe supports nested option literals as the above definition would imply. Your changes to the parser should naturally support this if your new pAggregateOptions definition is added to pConstant as a case.
[(foo_options) = { opt1: 123 opt2: "baz" opt3 {ioptA: true ioptB: "fie"} }]
Another example from googleapis; see specifically additional_bindings.
service Messaging {
rpc GetMessage(GetMessageRequest) returns (Message) {
option (google.api.http) = {
get: "/v1/messages/{message_id}"
additional_bindings {
get: "/v1/users/{user_id}/messages/{message_id}"
}
};
}
}
message GetMessageRequest {
string message_id = 1;
string user_id = 2;
}| @@ -618,19 +634,44 @@ module Parse = | |||
| <|> | |||
| (isProto3 >>. pStrMessageType_P3) | |||
|
|
|||
| let pRpcOptions = | |||
There was a problem hiding this comment.
Something about this section feels off. I'm pretty sure it can still be defined in terms of a pipe4, which to me better communicates the form of the TRPC production. Feels like there should be a common parser for all places where this type of option statement can occur.
Will try and look at this further when I get more time.
There was a problem hiding this comment.
Cleaned this up a bit and went back to a pipe4.
|
Thanks for your feedback! I might have time to look more at this in the next week. |
|
I changed the map to a option list which mostly is working, but one test fails because additional_bindings isn't followed by a colon. A quick search indicated that this is an array but I couldn't find any definition on how arrays should be parsed. |
|
@bhandfast Wow. Totally missed that in my one-line example. Edited my previous post to remove the ":" between "opt3" and "{", in order to be consistent with other examples on-line. The options block is written in Protobuf Text Format, but I couldn't find an official description... just this SO post. You might need to dig into the the Google protobuf source code, or maybe protobuf-c-text library for specifics. Yetch. Wish the protobuf spec was a bit more complete. |
|
Hmm, so essentially they use a different parser for options (data)? That's not something I was aware of. Maybe we need a completely new parser for protobuf text then? |
| let internal pConstant, internal pOptionDefR = createParserForwardedToRef<PConstant,State>() | ||
|
|
||
| let internal pColon = pstring ":" .>> ws | ||
| let internal pKey = manySatisfy (fun x -> isAsciiLetter x || x = '_') .>> ws |
There was a problem hiding this comment.
pKey can be deleted; should use pIdent or pIdent_ws instead (the latter two both parse a non-dotted identifier).
| between (str sOpen) (str sClose) | ||
| (ws >>. many1 (pElement .>> ws) |>> f) | ||
|
|
||
| let rec internal pKeyValue = pipe3 pKey pColon pConstant (fun k _ v -> k, v) |
There was a problem hiding this comment.
use pIdent_ws instead of pKey
|
Don't think we need another entire parser. There are a couple of issues, though, but I think they can be resolved by looking at the actual syntax that needs to be parsed. Best I can tell, the EBNF should look like the following:
There's a bit of subtly here. Note that aggregateBlock can include an empty statement, which handles constructs such as: The semicolon on the 2nd Finally, IIRC Let me know if you have trouble with this (or if I made a mistake, which is entirely possible :P ) |
|
I gave it another try now, but I'm not happy with the handling of empty statements. Maybe you can help with this? It feels like FParsec have another solution for that. |
|
I'm very excited to see progress here! I'm pushing for the adoption of gRPC in my company and this would help. I've now written a few services in Go that I'd love to be able to point an F# type provider at as the client. This is a step in that direction. |
|
@bhandfast Hmm, the empty statement business is a PITA. I took a crack at cleaning it up a bit and make sure it handles empty-statement-only, empty-at-beginning, multiple-empties-in-a-row, and empty-at-the-end. Also simplified the RPC parser a bit (went back to a See the attached patches: refactor-empty-statement-fixes.zip The main change is in Parser.fs, but do see the patches as I also updated the unit tests: Still fairly ugly, though. And I'm almost positive there are other places where a valid empty-statement will cause a parse error. This should work for now, but if anyone gets errors with valid empty-statements on other constructs, I/we may need to make more sweeping changes. Finally, I forgot to mark |
|
@bhandfast If you like, I could push these to your fork... just need write access... |
|
pAggregateBlockR can be simplified a bit by removing the superfluous |
|
Let's me to know when this gets to a point that you want to release it. I'll help get 0.4 out there. |
|
@jhugard I added you as a collaborator if you find the time to push the changes. It's late here now, I'll take a look at the changes tomorrow or something like that. |
|
@bhandfast Done. Pushed my changes (which also updated this PR). |
| betweenCurly | ||
| (sepBy pOption_ws (str_ws ",")) | ||
|
|
||
| let pRpcOptions = pRpcOptionsNew <|> pRpcOptionsOld |
There was a problem hiding this comment.
I'm not sure if betweenCurly consumes a token. If so, then this will need an attempt, as in:
let pRpcOptions = attempt pRpcOptionsNew <|> pRpcOptionsOld
We need a unit test for an rpc statement with old-style options to verify if this is needed or not.
|
@ctaggart Hold off a bit on the merge. Went over the code and (re)discovered I already did have empty statement handling in most places. I'd like to pull that out as an abstraction so it can be used (consistently and easily) everywhere. Will try and get to it later tonight... |
|
@jhugard Sounds good. |
…ty, add unit test for Service options, and fixed bugs related to parsing options.
|
@bhandfast Ok, done with refactoring to common handling of empty statements, via NOTE: the last check-in also eliminated what you were calling Otherwise, if it looks good to you, I'm OK with a merge. |
|
@jhugard Cool! I think you have more knowledge about the syntax then me. pRpcOptionsOld was more of a variable naming (a bad one) when I was messing around with the code at the beginning. If you think the code is ready now I think we should merge it. I haven't time to look at all the changes yet, but I will take a look at it when I find time later this week. |
… unit tests for consistency.
|
@ctaggart @bhandfast - Done and done. Went over all options usage, added unit tests (some of which failed), fixed another bug or two, added option support to @ctaggart - Can you update the |
|
Great work @jhugard! Thanks @bhandfast. Yes, I'll take a look at the readme. |
|
Big shout-out to @bhandfast! You made an excellent start on the alt options parsers, especially considering the poor docs from Google. Wouldn't have gotten done without your PR and assistance :-) Thanks a ton! |
An attempt to solve issues described in #60. I consider this a work in progress. Any suggestions are welcome as I'm new to FParsec.