hyperium
chore(interceptor)!: Rename Interceptor call method to intercept
Renames Interceptor's call method to intercept to explain the method's purpose.
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.
@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.
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!
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.
@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.
I am going to close this for now, we are going to be rethinking interceptors anyways, and I don't think this cosmetic change is worth it to merge right now.