-
Notifications
You must be signed in to change notification settings - Fork 1k
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
base: master
Are you sure you want to change the base?
chore(interceptor)!: Rename Interceptor call method to intercept #2006
Conversation
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. |
750bcce
to
fd433bb
Compare
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 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.
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. |
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.
I understand the status.
Understood, let's do that. |
Renames
Interceptor
'scall
method tointercept
to explain the method's purpose.