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

chore(interceptor)!: Rename Interceptor call method to intercept #2006

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tottoto
Copy link
Collaborator

@tottoto tottoto commented Oct 16, 2024

Renames Interceptor's call method to intercept to explain the method's purpose.

@LucioFranco
Copy link
Member

In my opinion we don't actually need this breaking change and it doesn't provide much value. I think for the moment we won't merge this.

@tottoto tottoto reopened this Dec 14, 2024
@tottoto tottoto force-pushed the rename-interceptor-call-method-to-intercept branch from 750bcce to fd433bb Compare December 14, 2024 08:00
@tottoto tottoto requested a review from LucioFranco December 14, 2024 08:01
@tottoto
Copy link
Collaborator Author

tottoto commented Dec 14, 2024

@LucioFranco

it doesn't provide much value

I don't think so. I have thought about this for a while, and my opinion remains that the benefits of this change is large.

The responsibility of Interceptor is to abstract intercepting, and it is fair to say that there is no benefit to obscuring the meaning of the API by calling it call, a word with a broader meaning, and that doing so does more harm than good. In other words, this change is a fix for improper method naming. Also, since the method name call is the same as that of the Service trait that provides the more general abstraction, it requires unnecessary care from implementers and readers of the code.

I don't think that only the need/necessity for the change is the right perspective. For any change we can find reasons why it is not needed/necessary. If we wonder whether we should make a change A->B, we can also ask ourselves whether we should make a change B->A. If the latter is rejected, that fact helps us to affirm the former. Applying this case, if we can determine that changing intercept to call and changing the trait content from an easy-to-understand method name to a more generic and ambiguous name is inappropriate, then we can understand this change to be appropriate.

The next release will be a breaking change, so it's a good opportunity for us to make improvements like this. It makes the code easier to understand and resolves the naming conflict with tower that tonic uses. The gains achieved without removing features are very large, and rejecting this kind of improvement because it seems unnecessary means missing opportunities to improve code quality not only this time but in the future. I don't think that's the future we want.

@shikhar
Copy link
Contributor

shikhar commented Dec 14, 2024

I wouldn't mind having to make this small code change for a breaking release.

Thank you for the tireless work you have been doing to improve the tonic codebase @tottoto!

@LucioFranco
Copy link
Member

I still do not think we should make this change. I know this has not been 100% public yet but I have been working with some folks in the core grpc team to rework some portions of tonic, mainly the transport layer. Due to this, I am not in favor of making proper long term breaking changes that are not high priority like this one. This is because I expect that we will change this api significantly with the new code. Just because we can make a breaking change does not mean we should. I do think your logic is correct @tottoto but to me the motivation for this name change is not strong enough to be something that I think is worth to break now and again in 6months.

The gains achieved without removing features are very large, and rejecting this kind of improvement because it seems unnecessary means missing opportunities to improve code quality not only this time but in the future. I don't think that's the future we want.

I totally agree, and we should have this mindset. What I would prefer to do though is with the new code coming that we focus on keeping tonic as it is now in a good shape security issues but try to reduce the amount of code churn. Shortly, probably starting in the new year, I expect that work on the "next" version of tonic (grpc-rust) will start with actual code. From this, lets formulate a plan on how we want to tackle the current code and proposed changes like this one or entirely reworking interceptors (something I am in favor of). This work will be public and we will provide time for feedback. @tottoto I will keep you in the loop as well, there is nothing major yet to report until the code starts flowing in but we are nearing end of the design phase.

Because of this, as of now I would like to not merge this and in favor consider doing this at later time.

@tottoto
Copy link
Collaborator Author

tottoto commented Dec 23, 2024

@LucioFranco

I know this has not been 100% public yet but I have been working with some folks in the core grpc team to rework some portions of tonic, mainly the transport layer. Due to this, I am not in favor of making proper long term breaking changes that are not high priority like this one.

If you think you want to postpone making breaking changes to the area for a while because you are working on developing codes which are affected by the changes to the area, I think that makes sense.

Shortly, probably starting in the new year, I expect that work on the "next" version of tonic (grpc-rust) will start with actual code.

I understand the status.

From this, lets formulate a plan on how we want to tackle the current code and proposed changes like this one or entirely reworking interceptors (something I am in favor of). This work will be public and we will provide time for feedback.

Understood, let's do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants