fx.Annotate: make variadic params optional by default #831

josephinedotlee · 2022-02-08T00:05:49Z

If annotate is passed a variadic function, the dependency
listed as the variadic parameter should be optional
unless otherwise specified with the optional:false
parameter tag. This commit addresses that bug.

Resolves GO-1169

If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug.

CLAassistant · 2022-02-08T00:05:54Z

All committers have signed the CLA.

codecov · 2022-02-08T00:06:07Z

Codecov Report

Merging #831 (f8878d1) into master (16d4edd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #831   +/-   ##
=======================================
  Coverage   98.36%   98.37%           
=======================================
  Files          29       29           
  Lines        1041     1043    +2     
=======================================
+ Hits         1024     1026    +2     
  Misses         11       11           
  Partials        6        6

Impacted Files	Coverage Δ
annotated.go	`99.39% <100.00%> (+<0.01%)`	⬆️

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 16d4edd...f8878d1. Read the comment docs.

annotated_test.go

sywhang · 2022-02-08T00:14:43Z

annotated_test.go

+
+		app := fxtest.New(t,
+			fx.Supply(
+				fx.Annotate(newB(newA())),


Looks like this has no annotation applied - why is this Annotated?

+1. I think fx.Annotate will no-op if there are no annotations.

Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>

abhinav

Let's add a test for the case where some parameters of the variadic function have annotations, but not all do.

For example, given,

func NewFoo(A, B, ...Option)

We should test at least:

fx.ParamTags(`name:"foo"`)  // only A has an annotation
fx.ParamTags(`optional:"true"`, `name:"bar"`)  // A and B have annotations
fx.ParamTags(`name:"foo"`, `optional:"true"`, `name:"whatever"`)  // everything has an annotation

abhinav · 2022-02-08T20:37:03Z

annotated.go

+		if i == ft.NumIn()-1 && ft.IsVariadic() && !strings.Contains(paramTag, "optional:") &&
+			!strings.Contains(paramTag, "group:") {
+			paramTag = paramTag + " optional:\"true\""
+		}


I feel like the string matching here will be fragile. Normally, since the format of struct tags is structured, we would talk about parsing it and checking, but that might be moot because:

Based on the issue description, we want to add optional:"true" only if the variadic parameter has no other tags. So the []Foo for whatever(...Foo) defaults to optional only if the user didn't explicitly specify anything for it.
So the check we actually need is: param tags for this argument is empty, or is too short to hit this argument.

Ping. This comment is still valid.

abhinav · 2022-02-08T20:37:27Z

annotated_test.go

+
+		app := fxtest.New(t,
+			fx.Supply(
+				fx.Annotate(newB(newA())),


+1. I think fx.Annotate will no-op if there are no annotations.

If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug.

Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>

This temporarily pins the Dig dependency in Fx to the master branch which has dig.Scope in preparation for adding fx.Module which is the corresponding user-facing API in Fx. In addition, this fixes a few tests to expect the new error message format that was changed with the graph refactoring PR in uber-go/dig#301.

This adds fx.Module Option which is a first-class object for supporting scoped operations on dependencies. A Module can consist of zero or more fx.Options. By default, Provides to a Module is provided to the entire App, but there is a room for adding an option to scope that to a Module. Module can wrap Options such asSupply/Extract, Provide, and Invoke but there are some Options that don't make sense to put under Module. For example, StartTimeout, StopTimeout, WithLogger explicitly errors out when supplied to a Module. Implementation-wise, a Module corresponds to dig.Scope which was added in uber-go/dig#305. Extra bookkeeping is done by the module struct which contains the provides and invokes to a Scope. Co-authored-by: Abhinav Gupta <mail@abhinavg.net> Co-authored-by: Abhinav Gupta <abg@uber.com> Co-authored-by: Abhinav Gupta <mail@abhinavg.net>

In #830, I reverted some of the code moves to make reviewing the change easier. This change adds back those moves on top of that PR. This reverts commit e4d006b. This reverts commit 8aa68c0. Co-authored-by: Abhinav Gupta <abg@uber.com>

annotated.go

Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>

annotated.go

annotated_test.go

…ariadics

abhinav · 2022-02-10T20:20:04Z

oops i broke it

sywhang

👍

fx.Annotate: make variadic params optional by default

f195ead

If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug.

sywhang reviewed Feb 8, 2022

View reviewed changes

josephinedotlee and others added 2 commits February 8, 2022 10:35

Update annotated_test.go

ab3ccde

Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>

Merge branch 'master' into annotate-optional-variadics

7ea7b90

abhinav reviewed Feb 8, 2022

View reviewed changes

josephinedotlee and others added 9 commits February 8, 2022 15:01

fx.Annotate: make variadic params optional by default

5a8abec

If annotate is passed a variadic function, the dependency listed as the variadic parameter should be optional unless otherwise specified with the `optional:false` parameter tag. This commit addresses that bug.

Update annotated_test.go

24c501b

Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>

fx.Module: Reorganize code (#832)

d9ebe58

In #830, I reverted some of the code moves to make reviewing the change easier. This change adds back those moves on top of that PR. This reverts commit e4d006b. This reverts commit 8aa68c0. Co-authored-by: Abhinav Gupta <abg@uber.com>

Update test to add appropriate annotations

9032ca9

Finish incomplete merge

3735ca9

Only make variadic params optional if no other tags are specified

5595e7e

remove print statements

16b497e

sywhang reviewed Feb 10, 2022

View reviewed changes

annotated.go Outdated Show resolved Hide resolved

Update annotated.go

1fbdc13

Co-authored-by: Sung Yoon Whang <sungyoonwhang@gmail.com>

josephinedotlee linked an issue Feb 10, 2022 that may be closed by this pull request

fx.Annotate: Variadic options should be optional #834

Closed

add extra test

96c8e4c

abhinav reviewed Feb 10, 2022

View reviewed changes

annotated.go Show resolved Hide resolved

annotated_test.go Show resolved Hide resolved

annotated_test.go Outdated Show resolved Hide resolved

abhinav added 3 commits February 10, 2022 12:14

Fix test "Provide variadic function with no optional params"

a987c12

Explain why we're adding the optional tag

2926652

Merge remote-tracking branch 'origin/master' into annotate-optional-v…

e0429a1

…ariadics

Fix test case, verified failing on master

f8878d1

abhinav approved these changes Feb 10, 2022

View reviewed changes

sywhang approved these changes Feb 10, 2022

View reviewed changes

josephinedotlee merged commit 9173ccd into master Feb 10, 2022

josephinedotlee deleted the annotate-optional-variadics branch February 10, 2022 20:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fx.Annotate: make variadic params optional by default #831

fx.Annotate: make variadic params optional by default #831

josephinedotlee commented Feb 8, 2022

CLAassistant commented Feb 8, 2022 •

edited

Loading

codecov bot commented Feb 8, 2022 •

edited

Loading

sywhang Feb 8, 2022

abhinav Feb 8, 2022

abhinav left a comment

abhinav Feb 8, 2022

abhinav Feb 9, 2022

abhinav Feb 8, 2022

abhinav commented Feb 10, 2022

sywhang left a comment

fx.Annotate: make variadic params optional by default #831

fx.Annotate: make variadic params optional by default #831

Conversation

josephinedotlee commented Feb 8, 2022

CLAassistant commented Feb 8, 2022 • edited Loading

codecov bot commented Feb 8, 2022 • edited Loading

Codecov Report

sywhang Feb 8, 2022

Choose a reason for hiding this comment

abhinav Feb 8, 2022

Choose a reason for hiding this comment

abhinav left a comment

Choose a reason for hiding this comment

abhinav Feb 8, 2022

Choose a reason for hiding this comment

abhinav Feb 9, 2022

Choose a reason for hiding this comment

abhinav Feb 8, 2022

Choose a reason for hiding this comment

abhinav commented Feb 10, 2022

sywhang left a comment

Choose a reason for hiding this comment

CLAassistant commented Feb 8, 2022 •

edited

Loading

codecov bot commented Feb 8, 2022 •

edited

Loading