Make skip-empty the default behavior and remove option
#40
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The plugin never generates empty scaffolding anymore. As such, we no longer need our custom matcher to validate the empty scaffolding content.
This is now the default behavior.
|
@danielmorrison I opted for a "safe" deprecation by introducing a warning this release if the flag is still present on the command line, and then error next release. I'm not entirely sure we need to do that. You could easily convince me to skip the warning altogether and just drop the flag entirely here. |
Rather than a soft deprecation here over two releases, we remove recognizing the flag entirely. The migration path is small/easy for this change (updating the command to simply remove the option), and we don't have a very large user base at the moment.
darronschall
changed the title
skip-empty the default behavior and deprecate optionskip-empty the default behavior and remove option
|
After consideration, decided to remove the flag entirely. The migration is simple. |
danielmorrison
approved these changes
May 28, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The default behavior of the protoc-gen-twirp_ruby go module is to generate empty scaffolding files for
.protofiles that do not contain any services. We chose to make our plugin behave similarly, for easier migration / backwards compatibility.We then introduced the
skip-emptyflag opt-in to avoid generating those empty files, in #21.At this point, we're ready to remove the flag and make this the default behavior. It's common for other twirp generators to do the same, see protoc-gen-twirp_typescript, protoc-gen-twirp-java, and protoc-gen-twirp_cpp.
Since the flag is removed as part of this change, passing it on the command line with cause the plugin to error. The path forward is to simply remove
--twirp_ruby_opt=skip-emptyfrom the command line if present.