-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: add feature resolution for protobuf editions #2029
Conversation
src/object.js
Outdated
var newValue = opt[name]; | ||
util.setProperty(newValue, propName, value); | ||
util.setProperty(newValue, propName, value, isFeature); |
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.
typo, the regex is only wrong if name
is the fully feature path. If it's always just features
, this will overwrite the entire features object for each specified feature right? It'll only keep the last one?
message Message { | ||
option features.json_format = LEGACY_BEST_EFFORT; | ||
oneof SomeOneOf { | ||
int32 a = 13; |
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.
you can put features in oneofs (same syntax as for message), do those work?
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.
Following the pattern of your other tests, I'd expect these two tests to be collapsed into one that tests both inheritance and overriding of message -> oneof
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.
isn't that tested in the test below? (feature resolution inheritance oneofs
)
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.
Kindof, it's not clear why this one is split into two when all of the other ones test both things in a single test
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.
ok I merged it into one test!
No description provided.