Skip to content
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

add method for adding synthetic fields to messages #47

Merged
merged 1 commit into from
Dec 7, 2015

Conversation

dmccartney
Copy link
Contributor

Hello @nicks, @koop,

Please review the following commits I made in branch 'daniel-synth-field'.

1cda2ff (2015-12-04 23:37:20 -0800)
add method for adding synthetic fields to messages

R=@nicks
R=@koop

*/
MessageDescriptor.prototype.addSyntheticField = function (type, name, tag) {
return this.addField(new FieldDescriptor(type, name, tag))
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an alternative I decided against:
add FieldDescriptor to the module exports so clients could create their own and use the existing addField(...).

That seemed like a big abstraction leak for little gain; plus, adding synthetic fields shouldn't be ordinary, so an explicit method seemed like a nice way to make that clear.

@nicks
Copy link
Contributor

nicks commented Dec 7, 2015

lgtm

i'm not sure i would have done this by modifying the proto model, but ok

dmccartney added a commit that referenced this pull request Dec 7, 2015
add method for adding synthetic fields to messages
@dmccartney dmccartney merged commit 669e9cf into master Dec 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants