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

feat(build): Add optional default unimplemented stubs #1344

Merged
merged 2 commits into from
Aug 14, 2023

Conversation

xanather
Copy link
Contributor

@xanather xanather commented Apr 4, 2023

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.

@xanather
Copy link
Contributor Author

xanather commented Apr 5, 2023

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.

@xanather
Copy link
Contributor Author

xanather commented Apr 17, 2023

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.

@NickLarsenNZ
Copy link

NickLarsenNZ commented Apr 17, 2023

@xanather

I'll fix the build formatting and clean up the commit history if so.

I assume you'll keep my contribution and not squash all of the commits?

@xanather
Copy link
Contributor Author

xanather commented Apr 18, 2023

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.

@NickLarsenNZ
Copy link

@LucioFranco can we please get this reviewed?
I had a look though the contributing guide and didn't see anything specific. Apologies for the distraction if we just need to wait in the PR queue.

@xanather
Copy link
Contributor Author

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.

@NickLarsenNZ
Copy link

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.

@xanather
Copy link
Contributor Author

xanather commented Apr 25, 2023

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.

@xanather
Copy link
Contributor Author

Please see write up at #1347.

Copy link
Member

@LucioFranco LucioFranco left a 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.

@LucioFranco LucioFranco added this pull request to the merge queue Aug 14, 2023
Merged via the queue into hyperium:master with commit aff1daf Aug 14, 2023
15 checks passed
@NickLarsenNZ
Copy link

Nice! So glad this is merged 🎉

@xanather
Copy link
Contributor Author

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.

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