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

Set oneof value to set field name #54

Merged
merged 1 commit into from
Jul 22, 2016
Merged

Conversation

kjvalencik
Copy link
Collaborator

Resolves #51

Previous generated code:

Envelope.read = function (pbf, end) {
    return pbf.readFields(Envelope._readField, {id: 0, int: 0, float: 0, string: ""}, end);
};
Envelope._readField = function (tag, obj, pbf) {
    if (tag === 1) obj.id = pbf.readVarint();
    else if (tag === 2) obj.int = pbf.readVarint();
    else if (tag === 3) obj.float = pbf.readFloat();
    else if (tag === 4) obj.string = pbf.readString();
};

New generated code:

Envelope.read = function (pbf, end) {
    return pbf.readFields(Envelope._readField, {id: 0, int: 0, float: 0, string: ""}, end);
};
Envelope._readField = function (tag, obj, pbf) {
    if (tag === 1) obj.id = pbf.readVarint();
    else if (tag === 2) obj.int = pbf.readVarint(), obj.value = "int";
    else if (tag === 3) obj.float = pbf.readFloat(), obj.value = "float";
    else if (tag === 4) obj.string = pbf.readString(), obj.value = "string";
};

@mourner
Copy link
Member

mourner commented Jul 22, 2016

We probably need to include the property in the initial literal so that its hidden class is not redefined when reading fields. Should it default to the first field then?

@kjvalencik
Copy link
Collaborator Author

It should probably be initialized as null. I don't think oneof means "at most one" not "exactly one".

@mourner
Copy link
Member

mourner commented Jul 22, 2016

@kjvalencik yeah, you're right. Let's add null.

BTW, I just invited you as a collaborator to the repo. Thanks for all the valuable contributions! Now you can create PR branches directly in the repo. The only rules are not to commit to master (always submit PRs for changes), and not to merge PRs unless reviewed and agreed upon.

@kjvalencik
Copy link
Collaborator Author

@mourner Is that the preferred method of opening a PR? Create a branch on mapbox/pbf and PR from that? Any particular naming schema for branches (e.g., kv/feature-foo)?

@kjvalencik
Copy link
Collaborator Author

Updated to initialize the oneof field with null.

@mourner
Copy link
Member

mourner commented Jul 22, 2016

Create a branch on mapbox/pbf and PR from that?

Yes.

Any particular naming schema for branches (e.g., kv/feature-foo)?

No, just use what's sensible — e.g. I usually use foo for features and fix-foo for bugs.

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.

None yet

2 participants