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

Added force_disable_services option to protobuf-ts/plugin for disabling service metadata generation #268

Merged

Conversation

ColinLaws
Copy link
Contributor

Summary of changes

Small change to introduce optionality in generating service metadata for proto messages. I have named the option disable_service_types.

Precaution

I am having difficulty understanding how to run tests after following the guide provided in CONTRIBUTING.md. I was not able to run tests successfully against packages/plugin, as I have an error that @protobuf-ts/plugin-framework is missing. I made sure to build and test the plugin-framework, but still I encounter issues when trying to run tests for the plugin package. Any help in contributing is appreciated, I'd like to make sure I'm not introducing bugs into this library. Thanks everyone.

@ColinLaws
Copy link
Contributor Author

ColinLaws commented Apr 15, 2022

Hey @timostamm I was wondering if the option name disable_service_types was okay with you. The reason I pluralized "types" and added some extra spacing between each word was for the sake of clarity while reading. I did the same for any variables/keys of objects, I tried to keep the code as cohesive as possible with the conventions that have already been established within the project.

@timostamm
Copy link
Owner

I am having difficulty understanding how to run tests after following the guide provided in CONTRIBUTING.md. I was not able to run tests successfully against packages/plugin, as I have an error that @protobuf-ts/plugin-framework is missing. I made sure to build and test the plugin-framework, but still I encounter issues when trying to run tests for the plugin package.

This is likely caused by lerna, the tool to link the packages within this monorepo. Please try to run make lerna-bootstrap in the root directory of the project. In general, just running make should runs all builds and all test, but you will need to have all build tools mentioned installed.

@tannera-cargill
Copy link

This is also a feature that we need, since we only are using protobuf-ts to generate the TS types and encoding/decoding functions.

Copy link
Owner

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

@ColinLaws, thanks for the update! I think we need to explain the option with a bit more detail, but that should be it.

packages/plugin/src/protobufts-plugin.ts Outdated Show resolved Hide resolved
Comment on lines +334 to +336
if (params?.force_disable_services) {
o.forceDisableServices = true;
}
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if we should set forcedServerStyle and forcedClientStyle here to be explicit, but can't think of a case the very obvious if-statement below wouldn't catch.

@timostamm timostamm merged commit b3f912c into timostamm:master Jun 16, 2022
timostamm added a commit that referenced this pull request Jun 16, 2022
@timostamm
Copy link
Owner

Released in v2.7.0.

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.

4 participants