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

#297 support optional parameters for thrift functions #298

Merged
merged 3 commits into from
Feb 26, 2025

Conversation

kubroid
Copy link
Contributor

@kubroid kubroid commented Jan 15, 2025

Resolves #297

@CLAassistant
Copy link

CLAassistant commented Jan 15, 2025

CLA assistant check
All committers have signed the CLA.

@kubroid kubroid changed the title support optional parameters for thrift functions #297 support optional parameters for thrift functions Jan 15, 2025
@kubroid kubroid marked this pull request as ready for review January 16, 2025 09:26
@kubroid kubroid requested review from a team as code owners January 16, 2025 09:26
@Millione
Copy link
Member

Millione commented Jan 20, 2025

@kubroid Hi, please sign the CLA first before we start the review!

@kubroid kubroid force-pushed the required_parameters_override branch from bf5b140 to 7ffe7ff Compare January 20, 2025 07:32
@kubroid
Copy link
Contributor Author

kubroid commented Jan 20, 2025

@Millione, done!

@PureWhiteWu
Copy link
Member

Is this a breaking change for existing users that we must bump the version?

@kubroid
Copy link
Contributor Author

kubroid commented Jan 20, 2025

In case using optional parameters it will break volo-build

@kubroid
Copy link
Contributor Author

kubroid commented Jan 20, 2025

Of course its on you to decide if version change is essential. Also I'm not sure about Cargo.lock changes are required.

@PureWhiteWu
Copy link
Member

Hello, I thought there's no one using this feature before, but I'm also not sure if we need to bump the minor version at this time.
Is depending on branch for you acceptable for you?

@kubroid
Copy link
Contributor Author

kubroid commented Jan 20, 2025

I guess... There is only one project requires this feature at this moment and I'm not sure if more will come in future.

@PureWhiteWu
Copy link
Member

Maybe we can just use branch at this moment? When there's more projects rely on this, we should have already published a new version lol.

@kubroid
Copy link
Contributor Author

kubroid commented Jan 20, 2025

Sure, no problems from my side :)

@PureWhiteWu
Copy link
Member

@kubroid Hi, could you please don't change the version, and then I think we can merge this?

@kubroid
Copy link
Contributor Author

kubroid commented Feb 17, 2025

@PureWhiteWu sorry guys, company decided to not go rust, so, all this effort is a waste of time :((
if you still interested in this feature, I'll update the PR.

PS: thanks a lot for your help, it was fun.

@PureWhiteWu
Copy link
Member

It's never a waste of time, thank you very much for helping us and the Rust open source community to be better.

Of course we'd like this pr to be merged, so could you please take some time to update this pr?

@kubroid kubroid force-pushed the required_parameters_override branch 2 times, most recently from c9db3b5 to e3f0a21 Compare February 23, 2025 18:27
@kubroid
Copy link
Contributor Author

kubroid commented Feb 23, 2025

@PureWhiteWu updated, please take a look.
I'm still not sure about if i need to commit a Carglo.lock changes.

@kubroid kubroid force-pushed the required_parameters_override branch from e3f0a21 to c62622f Compare February 25, 2025 10:17
@kubroid kubroid force-pushed the required_parameters_override branch from c62622f to b84377e Compare February 25, 2025 10:28
PureWhiteWu
PureWhiteWu previously approved these changes Feb 25, 2025
@PureWhiteWu
Copy link
Member

LGTM, thanks very much!

@Millione
Copy link
Member

@kubroid It seems that you should run UPDATE_TEST_DATA=1 cargo test again to pass CI.

@kubroid
Copy link
Contributor Author

kubroid commented Feb 25, 2025

@kubroid It seems that you should run UPDATE_TEST_DATA=1 cargo test again to pass CI.

thanks for the hint @Millione!
done

Copy link
Member

@Millione Millione left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much!

@Millione Millione merged commit 5c2c931 into cloudwego:main Feb 26, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Thrift service functions optional parameters support
4 participants