Skip to content

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