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

fx.Annotate: Support variadic functions #809

Merged
merged 8 commits into from
Nov 19, 2021
Merged

Conversation

xqbumu
Copy link
Contributor

@xqbumu xqbumu commented Nov 14, 2021

Add support to fx.Annotate for variadic functions.
The last parameter of variadic functions is now treated as a slice.
With this users can feed value groups into variadic functions.

Ref uber-go/dig#120

@CLAassistant
Copy link

CLAassistant commented Nov 14, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #809 (850af1d) into master (9377304) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #809   +/-   ##
=======================================
  Coverage   99.04%   99.05%           
=======================================
  Files          25       25           
  Lines         946      950    +4     
=======================================
+ Hits          937      941    +4     
  Misses          7        7           
  Partials        2        2           
Impacted Files Coverage Δ
annotated.go 99.28% <100.00%> (+0.02%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9377304...850af1d. Read the comment docs.

Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

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

Thanks for this change. Looks good to me

@xqbumu
Copy link
Contributor Author

xqbumu commented Nov 16, 2021

Thanks for this change. Looks good to me

There is something wrong that 'value groups cannot be optional' when use optional tags in ParamTags.
And that message is from dig where we can delve deeper.

Refer: uber-go/dig#120

@abhinav
Copy link
Collaborator

abhinav commented Nov 16, 2021

Hey, on a second look, this doesn't match the behavior of dig.
Dig specifically just ignores variadic arguments.
That is, if a function accepts variadic arguments, dig will fill everything except that.
I originally thought that's what this change was doing, but the test indicates otherwise.

If we want to support filling variadic arguments with value groups using this,
we can do that, but we should make that choice explicitly.
CC @sywhang @shirchen

RE: value groups cannot be optional:
They cannot be optional, in that you cannot tag them optional.
They are already optional, in that if no values are provided to that group, you get an empty slice.

@xqbumu
Copy link
Contributor Author

xqbumu commented Nov 17, 2021

Hey, on a second look, this doesn't match the behavior of dig. Dig specifically just ignores variadic arguments. That is, if a function accepts variadic arguments, dig will fill everything except that. I originally thought that's what this change was doing, but the test indicates otherwise.

If we want to support filling variadic arguments with value groups using this, we can do that, but we should make that choice explicitly. CC @sywhang @shirchen

RE: value groups cannot be optional: They cannot be optional, in that you cannot tag them optional. They are already optional, in that if no values are provided to that group, you get an empty slice.

Oh, I got it, very thanks for you explain.

@abhinav abhinav requested a review from r-hang November 19, 2021 18:48
@abhinav abhinav changed the title Support variadic function call fx.Annotate: Support variadic functions Nov 19, 2021
annotated_test.go Outdated Show resolved Hide resolved
annotated.go Outdated
// }, fx.ParamTags(``, `group:"server"`))
//
// If we provide the above to the application,
// any constructor in the Fx application can feed it HTTP handlers
Copy link
Contributor

@r-hang r-hang Nov 19, 2021

Choose a reason for hiding this comment

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

nit: it/its

Idea: Instead of 'feed' how about 'inject' or can inject its HTTP handlers with fx.Annotate, fx.Annotated, or fx.Out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ack

abhinav and others added 2 commits November 19, 2021 12:27
Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>
Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

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

LGTM

@abhinav abhinav merged commit 0c1b3fe into uber-go:master Nov 19, 2021
@abhinav
Copy link
Collaborator

abhinav commented Nov 19, 2021

Thanks! @xqbumu

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.

5 participants