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

Add documentation and tests for soft group values #909

Merged
merged 5 commits into from Aug 4, 2022
Merged

Conversation

EstebanOlmedo
Copy link
Collaborator

This adds documentation regarding soft group values feature and also
adds test cases to check this feature works with modules, Supply,
Decorate and Annotate.

@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #909 (6c2926e) into master (d88feb8) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #909   +/-   ##
=======================================
  Coverage   98.71%   98.71%           
=======================================
  Files          30       30           
  Lines        1328     1328           
=======================================
  Hits         1311     1311           
  Misses         11       11           
  Partials        6        6           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

annotated_test.go Outdated Show resolved Hide resolved
),
fx.Provide(
fx.Annotate(
func() (string, int) { return "mushroomburger", 35 },
Copy link
Contributor

Choose a reason for hiding this comment

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

non-review comment: mushroom burger...? 🤣

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While I was writing this I couldn't remember the name of another burger hahaha

inout.go Outdated
Comment on lines 168 to 170
// will be populated only with values from already-executed constructors, this
// means constructors of soft value groups will be called only if they provide
// another requested value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// will be populated only with values from already-executed constructors, this
// means constructors of soft value groups will be called only if they provide
// another requested value.
// will be populated only with values from already-executed constructors.

The last statement isn't quite true. If there's another function consuming the value then it will still populate it.

It's better to describe soft value groups as specifying the parameter as an 'opportunistic' consumer - if the values are already provided in the DI container, it consumes them. Otherwise, it doesn't.

You also want to specify that this is only a valid option for input parameters in the docs.

inout.go Outdated
Comment on lines 175 to 176
// Handlers []Handler `group:"server"`
// Logger *zap.Logger
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation seems off here

inout.go Outdated
Comment on lines 182 to 185
// When `NewHandlerAndLogger` and `NewHandler`are provided, and `Foo` invoked,
// the only constructor called is `NewHandler`, because this also provides
// `*zap.Logger` needed in the `Params` struct.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Apart from this example, can you also provide an example where:

  1. If you have a constructor for a type, and the only consumer is the soft value group, it is not populated.
  2. In the exact same case above, add a consumer that also consumes that type. Then show that it is populated.

module_test.go Show resolved Hide resolved
@sywhang
Copy link
Contributor

sywhang commented Aug 4, 2022

Also it looks like you have a merge conflict. You might want to rebase master :)

EstebanOlmedo and others added 3 commits August 4, 2022 14:33
This adds documentation regarding soft group values feature and also
adds test cases to check this feature works with modules, `Supply`,
`Decorate` and `Annotate`.
This adds documentation regarding soft group values feature and also
adds test cases to check this feature works with modules, `Supply`,
`Decorate` and `Annotate`.
@sywhang sywhang merged commit 1124297 into master Aug 4, 2022
@sywhang sywhang deleted the soft-group-values branch August 4, 2022 22:17
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.

None yet

2 participants