set and clear methods return message instance for method chaining

improbable-eng / ts-protoc-gen

Protocol Buffers Compiler (protoc) plugin for TypeScript and gRPC-Web.

Apache License 2.0

1.36k stars 172 forks source link

set and clear methods return message instance for method chaining #248

Closed kcirtaptrick closed 3 years ago

kcirtaptrick commented 3 years ago

Changes

set and clear methods return their parent message instance instead of void

Verification

TSLint and tests passed, tried generating files in a project and they're working properly.

kcirtaptrick commented 3 years ago

/assign

mohammadavood commented 3 years ago

please merge this feature. method chaining is critical and important

MarcusLongmuir commented 3 years ago

Hi both. Apologies for the delay on this. This is significant change to the types and needs tests to improve confidence that the types match the runtime behaviour.

Could you please:

Update the generate.js file to use the latest version of protoc (the currently-used version doesn't have chaining).
Add tests for chaining and assert the return types and values of the set functions.
Add chaining to the add functions. From the docs and the protoc write code the add functions for primitive repeated fields also return the instance, but add functions for non-primitives appear to return the added value as per the protobuf reference:

addFoo(): Appends a value of foo to the end of the list of foos that was in the message. Returns the outer message for chaining only if the added value was a primitive. For added messages, returns the message that was added

improbable-prow-robot commented 3 years ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: To complete the pull request process, please assign moadz You can assign the PR to them by writing /assign @moadz in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/improbable-eng/ts-protoc-gen/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

kcirtaptrick commented 3 years ago

Just made 2 of the changes, how would I go about wriitng tests for this? Would I create a new integration test or add to one of the existing ones?

kcirtaptrick commented 3 years ago

Closing this PR since I've since moved to protobuf.js and no longer need this change. If someone else wants to continue this effort, feel free