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 depinject codegen #12556

Closed
2 tasks
aaronc opened this issue Jul 13, 2022 · 4 comments
Closed
2 tasks

Add depinject codegen #12556

aaronc opened this issue Jul 13, 2022 · 4 comments
Assignees
Labels
C:depinject Issues and PR related to depinject

Comments

@aaronc
Copy link
Member

aaronc commented Jul 13, 2022

In our discussions around app wiring, we agreed that depinject should generate code for dependency provisioning so that it is easier to review the dependency graph in PR reviews and so that appconfig/depinject are fully opt-in/opt-out.

Design Overview

The proposed design to do injection by reflection first and generating the code during the reflection process. We will require a certain template file that needs to be run to trigger codegen (which is signaled by the depinject.Codegen debug option):

//+build depinject // this build tag is used to separate the codegen template from the generated code

// this must be a static variable for depinject to use it for codegen
var appConfig depinject.Config = appconfig.LoadYaml(...)

// codegen template functions must follow a strict pattern or depinject will report an error
// all function parameters are runtime supplied dependencies, and all outputs are dependencies we're trying to resolve
func Build(appOpts servertypes.AppOptions) (appBuilder *runtime.AppBuilder, error) {
  err = depinject.InjectDebug(
    depinject.Codegen(), // the presence of this option tells depinject that the calling function is a codegen template and that codegen should be run
    depinject.Configs(
         appConfig, // 1 or more static vars are allowed here
         depinject.Supply(appOpts) // all function parameters must be specified here
    ), 
    &appBuilder, // all function return values (beside error) must be specified here
  )
  return
}

If this file were called build.go, depinject would generate a file build.depinject.go.

TODO

  • require that all provider/invoker functions and their input/output params are exported
  • probably unexport ProviderDescriptor
@aaronc aaronc changed the title depinject codegen Add depinject codegen Jul 14, 2022
@kocubinski
Copy link
Member

What action does a user take to generate build.depinject.go? Is go build -tags depinject enough?

@aaronc
Copy link
Member Author

aaronc commented Jul 14, 2022

What action does a user take to generate build.depinject.go? Is go build -tags depinject enough?

They need to have some code that calls Build. I'm actually calling Build in a test file also with go:build depinject and then calling go test -tags depinject.

@aaronc aaronc self-assigned this Jul 14, 2022
mergify bot pushed a commit that referenced this issue Jul 15, 2022
## Description

Ref #12556 

To make things easier I've starting splitting up #12469 into smaller PRs.

This first PR for codegen adds basic infrastructure for:
* adding package imports
* creating new var names (`*ast.Ident`) without conflicts

Other PRs will add the actual codegen functionality.



---

### 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/main/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/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/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)
@julienrbrt
Copy link
Member

I think you mentioned that already, but we will definitely need something to improve the UX, so we can just do a make deps or smth. This is a bit unintuitive to have to run tests beforehand (at least for people used to wire).

@ilgooz
Copy link
Contributor

ilgooz commented Jul 28, 2022

cc @aljo242

mergify bot pushed a commit that referenced this issue Aug 1, 2022
## Description

Ref #12556 

This PR continues with basic codegen infrastructure for depinject, this time adding the ability to generate `ast.Expr`'s for `reflect.Type` and `reflect.Value`.



---

### 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/main/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/main/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/main/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)
@tac0turtle tac0turtle added the C:depinject Issues and PR related to depinject label Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:depinject Issues and PR related to depinject
Projects
None yet
Development

No branches or pull requests

5 participants