-
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
feat(build): Add optional default unimplemented stubs #1344
Conversation
Let me know if this has any chance to get merged. I'll fix the build formatting and clean up the commit history if so. Arguments for this feature are already laid out in #221. |
Thanks for improving my patch @NickLarsenNZ and writing out some doco. Returning default values as another option is a good idea. I have merged your PR into mine. |
I assume you'll keep my contribution and not squash all of the commits? |
Yep, worse case I will squash mine and cherry pick yours on-top so your contribution will stay. Waiting comment from any of the maintainers. |
@LucioFranco can we please get this reviewed? |
Hey Nick, I had a quick chat with Lucio on Discord, he's been super busy lately and finds this feature low priority. I'm going to simplify the patch back to just a Boolean flag with no map and write some tests which would have a higher chance of being merged. I think having an overridable map should be a seperate issue. |
No problem, I'll raise a new one. In either case, your contribution is helpful, thanks. |
Force pushed removing map feature by Nick, and also added test suite for the boolean feature. Now that I was forced to properly implement and validate this via tests, the streaming functionality using the older implementation was incorrect @NickLarsenNZ, have another look. @LucioFranco this is ready for review now. I'm quite happy with it other than what I had to do with defining an explicit streaming type on server requests (if the feature is enabled, we default to BoxStream). Without this adding a streaming RPC to a gRPC model will be a breaking change for code generation and rust compiler will complain due to lack of concrete type. |
Please see write up at #1347. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good! Good work @xanather this will be included in the next release (0.10). I appreciate the patience.
Nice! So glad this is merged 🎉 |
Oh cool, thanks for merging, I thought it wasn't going in due to your comment on #1347 Definitely should be revisited once breaking changes are in order. |
Motivation
Enables functionality pre #221 without changing the current default functionality
Solution
Add it as a builder option.
Other notes
I agree that by default unimplemented stubs should not be added due to Rust being a strict language. However I think the option should still be available for backwards-compatibility.