-
Notifications
You must be signed in to change notification settings - Fork 349
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
feat(plugin): optional fields for oneof properties #549
base: main
Are you sure you want to change the base?
Conversation
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.
Great, thanks for the PR @Larkooo !
@@ -655,134 +655,134 @@ export const protoMetadata: ProtoMetadata = { | |||
path: [4, 0], | |||
span: [50, 0, 53, 1], | |||
leadingComments: | |||
' Wrapper message for `double`.\n\n The JSON representation for `DoubleValue` is JSON number.\n', | |||
' Wrapper message for `double`.\r\n\r\n The JSON representation for `DoubleValue` is JSON number.\r\n', |
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.
@Larkooo FWIW these changes (and the *.bin
file changes) might be from running proto2bin:local
instead of the docker-based proto2bin
?
We used to have a lot of churn on these files from contributors having different versions of protoc
installed on their machines, but in theory the docker-based setup that @boukeversteegh put in place should have removed this churn.
Can you try the docker-based commands and see if it resolves these differences? Or were you already doing that, and we still have some non-determinism issues?
@@ -0,0 +1,6831 @@ | |||
Arguments: |
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.
Seems that this file got included in the commit accidentally
I think it might be worthwhile to add a simple test that only instantiates an object of a type with one-of fields, and omits a field. If we ever break this feature, the TS file won't compile. for example, in oneof-properties-tests: it('allows omitting fields', () => {
let msg: PleaseChoose = {
name: 'foo',
}
}); |
cdf2835
to
7017d4c
Compare
Hi @Larkooo, very strong work! Are you planning to continue working on it? |
Just ran into this issue today. Anything I can do to help get the PR merged? |
@scott-lc yeah, would be great if you wanted to pick this up; just scanning the PR, I think the issues would be to: a) See the PR comment about running b) Remove the c) Rebase the PR onto the latest d) Update one of the Thanks! |
addresses issue #548