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

Update generated service and client class names to better handle well-named services. #97

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

darronschall
Copy link
Contributor

This PR follows #91 and subsequent discussions.

When a service is well-named and ends in "Service", such as "MessagesService", the generator generates class MessagesServiceService < Twirp::Serivice and class MessagesServiceClient < Twirp::Client.

This change causes the generator to generate class MessagesService < Twirp::Serivice and class MessagesClient < Twirp::Client instead.

The service name passed to the service DSL is not impacted (which was the problem in #91 that needed a revert).

For backwards compatibility, this change does not impact the current hello_world example, as demonstrated by:

# Check out and build the generator using updates from the this branch
$ git checkout service-class-name-suffix-fix
$ cd ./protoc-gen-twirp_ruby 
$ go build
$ go install

# Run code gen
$ cd ../example 
$ protoc --proto_path=. ./hello_world/service.proto --ruby_out=. --twirp_ruby_out=.

# Demonstrate no changes for services not already well-named
$ git status
On branch service-class-name-suffix-fix
Your branch is up to date with 'origin/service-class-name-suffix-fix'.

nothing to commit, working tree clean

I'm not familiar enough with go and testing to write tests for this in main_test.go. Any help there is appreciated.

…es are not properly named.

Protobuf services should end in `Service`. This is a [buf lint rule](https://github.com/bufbuild/buf-examples/blob/main/linting/bad/acme/weather/v1/weather.proto#L39), recommended via [the protobuf style guide](https://developers.google.com/protocol-buffers/docs/style#services), and demonstrated in [Twirp Best Practices](https://twitchtv.github.io/twirp/docs/best_practices.html))

This change avoids generating class names such as `MessagesServiceService`.
…vices.

Similar to 5322b13, for well-named services that end in "Service", this change prevents the generated clients from being named like "MessagesServiceClient".
@darronschall
Copy link
Contributor Author

@arthurnn Should I also update names in test to be well-named, like #91 did in 8125bf0? Separate PR maybe?

@darronschall darronschall marked this pull request as ready for review January 6, 2023 15:04
@arthurnn
Copy link
Owner

thanks @darronschall , I have this on my schedule to test against some local projects. At first glance it looks good.
I will give it a final look by next week and let you know.

thanks again for the contribution.

@arthurnn
Copy link
Owner

I am putting a pin on this until I figure out what to do about the latest version hiccups. see #99.

thanks, and I added this back to my backlog to review by the end of the week

@darronschall
Copy link
Contributor Author

@arthurnn Just wanted to check in since it's been a few months. Have you been able to take a look at this yet? No rush, and no pressure. It'd be nice to land this at some point.

@danielmorrison
Copy link
Contributor

@arthurnn any chance we can get some eyes on this? It continues to be an annoyance for us.

Happy to assist with review and/or maintenance if it'd help.

@tiwilliam
Copy link

This would be a nice improvement. We run in to these when trying to respect service name lint rules.

@danielmorrison
Copy link
Contributor

To clarify the problem again, when I have a service that ends in Service, it ends up duplicating the word Service in my _twirp.rb generated file:

service MessagesService {
class MessagesServiceService < ::Twirp::Service

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