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

Fix protoc-gen-twirp-es.ts in the protoplugin-example package #367

Merged
merged 3 commits into from
Jan 20, 2023

Conversation

s-hakase
Copy link
Contributor

Fix the following bugs in the sample plugin

  • The code to output closing brackets was incorrectly positioned, causing unnecessary files to be generated from protos where no RPCs existed.
  • Incorrect positioning of code outputting closing brackets was generating incorrect syntax when multiple RPCs existed.
  • Unnecessary spaces were being output.

@CLAassistant
Copy link

CLAassistant commented Jan 19, 2023

CLA assistant check
All committers have signed the CLA.

@@ -76,10 +76,10 @@ function generateTs(schema: Schema) {
f.print(" ", method.output, ".fromJson(data as ", JsonValue, ")");
f.print(" );");
}
f.print(" }");
Copy link
Member

Choose a reason for hiding this comment

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

Actually this should be moved up even further inside the if (method.methodKind === MethodKind.Unary) { condition. Otherwise if you have only streaming services, it will print an extra curly brace. Can you move up inside that if statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review.
It was indeed as you said.
I have corrected it, please check again.

@s-hakase s-hakase requested a review from smaye81 January 20, 2023 02:29
@smaye81
Copy link
Member

smaye81 commented Jan 20, 2023

Looks great. Thanks for the fix @s-hakase!

@smaye81 smaye81 merged commit 736521b into bufbuild:main Jan 20, 2023
This was referenced Feb 28, 2023
smaye81 added a commit that referenced this pull request Mar 2, 2023
This release includes the following:

## Enhancements
* Support size-delimited messages by @timostamm in #387.

## Bug Fixes
*  Upgrade to Protobuf v22.0 by @smaye81 in #394.
* Fix type declaration emitting when using NodeNext module resolution by
@fubhy in #398.
*  Strip rewrite_imports parameter by @smaye81 in #386.

## New Contributors
* @s-hakase made their first contribution in #367.
* @balzdur made their first contribution in #368.
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.

3 participants