-
Notifications
You must be signed in to change notification settings - Fork 48
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
Use jsonpb for json unmarshalling and interface for oneof #13
base: master
Are you sure you want to change the base?
Conversation
PS @awalterschulze did you see the email I sent you? |
|
Yes I received your email, I will be replying to it tonight :) Thank you very much for the reply. |
I will let you know once I have completed the update of gogoprotobuf |
Ok cool!
No, 3 is actually the largest part. The interface should reflect the fact that only one field can be set. The json creation should include only the selected field. Currently when using a oneof definition like this oneof foo {
int64 a = 1;
int64 b = 2;
} And in the interface {"a": 42, "b": 0}
So there are some modifications to the generated html/javascript that I'm currently working on. I'll add it to this PR when it's done. Together with your fix for 2 we'll have |
Ok now I understand this is only the first part of multiple pull requests, my bad. |
Yes I'll add the commit for html/javascript to this PR. Opened this as a work-in-progress PR so you could see what direction I'm going and give help/feedback. |
Excellent :) |
0dd2fe6 adds support for oneof in the generated go/javascript. |
@@ -685,6 +711,9 @@ func Builder(visited map[string]struct{}, root bool, fieldname string, help stri | |||
s = append(s, `s += '<a href="#" class="del-child btn btn-danger btn-xs" role="button" fieldname="`+fieldname+`">Remove</a>'`) | |||
s = append(s, `s += '</div><div class="col-sm-10">'`) | |||
s = append(s, `s += '<label class="heading">`+fieldname+`</label>'`) | |||
if len(msg.OneofDecl) > 0 { | |||
s = append(s, `s += '<br/>This message has `+fmt.Sprintf("%d", len(msg.OneofDecl))+` oneof declarations.<br/>'`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is just for debugging, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, I wanted to include an explanation. But maybe it's better to put some time in the layout itself so the user can easily identify which oneof
fields belong together.
Maybe you could edit https://github.com/gogo/letmegrpc/blob/master/letmetestserver/serve/serve.proto to include a oneof field so I could run it and get an idea of what it looks like? |
Good idea, will do. |
Added an example to /Label/Loop |
I've detected that this will only work with basic types, not yet with a |
gogoprotobuf is updated again, but only up to golang/protobuf@4a63085, since I have found some issues with this commit. |
Can you run your version? |
Yes, since I updated to the latest version of gogoprotobuf mine does too.. Maybe something changed upstream causing it to fail? |
It doesn't seem to have anything to do with your changes. I'll take a deeper look. |
I have an idea it has to do with the writeOutput optimization when I only use the GeneratePlugin method. |
fixed :) |
I finally took a look at your gui :) I think there are quite a few ways to do oneof, but I don't know which will be the best.
I think the best way would make it very obvious to the user that there is one field to fill in. |
@awalterschulze I want to add zebra colors (odd/even) to oneof's belonging together. That should make it clear when there are multiple oneof groups. I think showing multiple fields is a better way to represent oneof, its closer to the underlying encoding. As scope is quite difficult to maintain in the generate javscript, I've made the oneof purely based on domnode classes. This means that supporting messages as a oneof field will be quite difficult, it wasn't part of my original plan to add oneof. |
I really like the idea of zebra colors and showing the multiple fields does keep things easier :) |
Here is an example message.
|
I got the idea from a colleague, but maybe we could use tabs
|
d696d53
to
64a18ca
Compare
I've found a way to get oneof working.
Fixes #10.
jsonpb
won't detect the oneof fields. I'm not sure if using go_out instead of gogo_out breaks other things?? So far it seems to be working fine.