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.Module: Reorganize code #832

Merged
merged 2 commits into from
Feb 8, 2022
Merged

fx.Module: Reorganize code #832

merged 2 commits into from
Feb 8, 2022

Conversation

sywhang
Copy link
Contributor

@sywhang sywhang commented Feb 8, 2022

In #830, we 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.

In uber-go#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.
@sywhang sywhang requested a review from abhinav February 8, 2022 18:12
@abhinav
Copy link
Collaborator

abhinav commented Feb 8, 2022

missed sywhang@7f1a2c7

@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #832 (3f4510d) into master (ba16de2) will decrease coverage by 0.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #832      +/-   ##
==========================================
- Coverage   98.53%   98.32%   -0.22%     
==========================================
  Files          26       28       +2     
  Lines        1021     1012       -9     
==========================================
- Hits         1006      995      -11     
- Misses         10       11       +1     
- Partials        5        6       +1     
Impacted Files Coverage Δ
app.go 94.88% <ø> (-2.43%) ⬇️
invoke.go 100.00% <100.00%> (ø)
module.go 100.00% <100.00%> (ø)
provide.go 100.00% <100.00%> (ø)

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 ba16de2...3f4510d. Read the comment docs.

@sywhang sywhang merged commit d2c3431 into uber-go:master Feb 8, 2022
@sywhang sywhang deleted the reorganize branch February 8, 2022 18:36
josephinedotlee pushed a commit that referenced this pull request Feb 8, 2022
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 <[email protected]>
josephinedotlee added a commit that referenced this pull request Feb 10, 2022
* fx.Annotate: make variadic params optional by default

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

Co-authored-by: Sung Yoon Whang <[email protected]>

* fx.Annotate: make variadic params optional by default

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

Co-authored-by: Sung Yoon Whang <[email protected]>

* Temporarily pin Dig dependency to master (#827)

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.

* Add fx.Module (#830)

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 <[email protected]>
Co-authored-by: Abhinav Gupta <[email protected]>
Co-authored-by: Abhinav Gupta <[email protected]>

* fx.Module: Reorganize code (#832)

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 <[email protected]>

* Finish incomplete merge

* Only make variadic params optional if no other tags are specified

* remove print statements

* Update annotated.go

Co-authored-by: Sung Yoon Whang <[email protected]>

* add extra test

* Fix test "Provide variadic function with no optional params"

* Explain why we're adding the optional tag

* Fix test case, verified failing on master

Co-authored-by: Sung Yoon Whang <[email protected]>
Co-authored-by: Sung Yoon Whang <[email protected]>
Co-authored-by: Abhinav Gupta <[email protected]>
Co-authored-by: Abhinav Gupta <[email protected]>
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.

2 participants