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

feat: add low-level dependency injection container API #9658

Merged
merged 17 commits into from
Aug 12, 2021

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Jul 9, 2021

Description

closes #9774

This PR defines the low-level API for the dependency injector container to be used for the new app.go wiring discussed in #9182, demoed in #9529, and refined by the App Wiring Working Group.

To see example usage, see container_test.go. All tests are expected to fail as there is no implementation here, just API. Thus the tests are skipped.

This API is inspired by https://github.com/uber-go/dig and https://github.com/uber-go/fx and is intended to be for low-level components, with a higher-level Module infrastructure sitting on top of this.

The container package here is defined as a standalone go module so that this code-base is isolated and self-contained and concerned with doing one thing (dependency injection) well.


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage
  • manually tested (if applicable)

@aaronc aaronc changed the title feat: add dependency injection container API feat: add low-level dependency injection container API Jul 9, 2021
@aaronc aaronc marked this pull request as ready for review July 22, 2021 19:27
@aaronc aaronc requested a review from alexanderbez as a code owner July 22, 2021 19:27
@aaronc
Copy link
Member Author

aaronc commented Jul 22, 2021

Note that we will need to add github actions to cover all nested go modules (as in regen-network/regen-ledger#357). @cyberbono3 would you be able to support with that?

// directly, but will instead use the struct's fields to resolve or populate
// dependencies. Types with embedded StructArgs can be used in both the input
// and output parameter positions.
type StructArgs struct{}
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought it was simpler to have one struct vs dig.In and dig.Out... Wonder if anyone has an opinion

Copy link
Collaborator

Choose a reason for hiding this comment

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

In / Out makes it more clear when reading a structure definition, without going into the provider / usage:

  • when I see a struct withdig.In I quickly associate it with a dependencies of some type.
  • when I see a struct with dig.Out I see that those are some dependencies a provider can provide.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy to change it to In/Out, but note that I also have structs named Input and Output here to describe the actual constructor inputs/outputs... Any suggestions on naming?

@aaronc aaronc added the C:depinject Issues and PR related to depinject label Jul 26, 2021
@aaronc aaronc removed the C:depinject Issues and PR related to depinject label Jul 26, 2021
@github-actions github-actions bot added the T: CI label Jul 26, 2021
@aaronc aaronc added the C:depinject Issues and PR related to depinject label Jul 26, 2021
@codecov
Copy link

codecov bot commented Jul 26, 2021

Codecov Report

Merging #9658 (d553263) into master (0bcce5d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9658   +/-   ##
=======================================
  Coverage   63.60%   63.60%           
=======================================
  Files         568      568           
  Lines       53509    53511    +2     
=======================================
+ Hits        34034    34037    +3     
+ Misses      17550    17549    -1     
  Partials     1925     1925           
Impacted Files Coverage Δ
store/types/gas.go 84.44% <ø> (ø)
store/types/store.go 68.75% <ø> (ø)
x/upgrade/types/plan.go 96.29% <ø> (ø)
x/upgrade/abci.go 94.73% <100.00%> (+0.14%) ⬆️
x/upgrade/keeper/keeper.go 80.18% <100.00%> (+0.08%) ⬆️
crypto/keys/internal/ecdsa/privkey.go 84.21% <0.00%> (+1.75%) ⬆️

Copy link
Member

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

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

Approval meant to move things forward as follow-up PRs have already been opened

Copy link
Contributor

@blushi blushi left a comment

Choose a reason for hiding this comment

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

Approving, maybe we can try to start some live code review of #9666 in the next WG call then?

// directly, but will instead use the struct's fields to resolve or populate
// dependencies. Types with embedded StructArgs can be used in both the input
// and output parameter positions.
type StructArgs struct{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

In / Out makes it more clear when reading a structure definition, without going into the provider / usage:

  • when I see a struct withdig.In I quickly associate it with a dependencies of some type.
  • when I see a struct with dig.Out I see that those are some dependencies a provider can provide.

@aaronc
Copy link
Member Author

aaronc commented Aug 11, 2021

How about let's merge. I can do a live walkthrough of the next PR tomorrow, and we can discuss any naming details we want to tweak.

@aaronc aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 11, 2021
@aaronc aaronc merged commit e656d5e into master Aug 12, 2021
@aaronc aaronc deleted the aaronc/container-init branch August 12, 2021 13:07
mergify bot pushed a commit that referenced this pull request Oct 4, 2021
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description

closes #9775 
needs #9658

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass. C:depinject Issues and PR related to depinject T: CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Dependency Injection Container API
5 participants