Skip to content

Comments

fix(transport/grpc): add client option to disable prefix#523

Closed
terinjokes wants to merge 1 commit intogo-kit:masterfrom
terinjokes:terin/grpctransport-without-prefix
Closed

fix(transport/grpc): add client option to disable prefix#523
terinjokes wants to merge 1 commit intogo-kit:masterfrom
terinjokes:terin/grpctransport-without-prefix

Conversation

@terinjokes
Copy link
Contributor

@terinjokes terinjokes commented May 8, 2017

Add a ClientOption to revert the auto-prefixing of "pb" to service names
when there's no other prefix already specified.

Relates to #447.


This implementation touches the fewest lines of code, at the expense at perhaps being a bit more complex. An alternative implementation I have in mind avoids the fmt.Sprintf on line 45 to construct the method, stores them separately, until the method is actually needed to Invoke. The ClientOption could then just set a flag to turn off the prefixing.

What do you think?


Additionally, is there something I can do to denote that auto-prefixing is deprecated, so we can re-evaluate this in a future breaking change?

Add a ClientOption to revert the auto-prefixing of "pb" to service names
when there's no other prefix already specified.

Relates to go-kit#447.
@basvanbeek
Copy link
Member

Hi @terinjokes thanks for this. Contrary to what we discussed earlier I'm leaning towards biting the bullet and removing the auto prefixing of pb. if no namespace was given.

We postponed Go kit v1.0.0 and are still in 0.x.x so have no hard API stability guarantees yet. Changing existing gRPC clients which break due to this should be easy.

@peterbourgon what do you think?

@peterbourgon
Copy link
Member

No objections to breaking the API. I don't have sufficient context to have an opinion on the prefixing or not; I defer to your judgment there.

@terinjokes
Copy link
Contributor Author

Presumably people are vendoring and they can fix the prefixes on their own schedule.

@basvanbeek If you give me a bit to finishing waking up and starting to work, I don't mind sending a PR to remove it.

@terinjokes
Copy link
Contributor Author

@basvanbeek I opened alternative CL #524

@basvanbeek
Copy link
Member

closed in favor of #524

@basvanbeek basvanbeek closed this May 9, 2017
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