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

Support hooks on embed:"" fields #493

Merged
merged 1 commit into from
Jan 29, 2025

Conversation

abhinav
Copy link
Contributor

@abhinav abhinav commented Jan 28, 2025

(I realize that I opened this without discussing the feature first,
so if you don't think this feature makes sense, feel free to close this PR.
I've tried to outline the reasoning below.)


Relates to 840220c (#90)

This change adds support for hooks to be called on fields
that are tagged with embed:"".

Use case

If a command has several subcommands,
many (but not all) of which need the same external resource,
this allows defining the flag-level inputs for that resource centrally,
allowing subcommands to declare their dependency on these resources
and use them in their Run in a highly composible manner.

For example, imagine:

type githubClientProvider struct {
    Token string `name:"github-token" env:"GITHUB_TOKEN"`
    URL   string `name:"github-url" env:"GITHUB_URL"`
}

func (g *githubClientProvider) BeforeApply(kctx *kong.Context) error {
  return kctx.BindToProvider(func() (*github.Client, error) {
    return github.NewClient(...), nil
  })
}

Then, any command that needs GitHub client will add this field,
any other resource providers it needs,
and add parameters to its Run method to accept those resources:

type listUsersCmd struct {
    GitHub githubClientProvider `embed:""`
    S3     s3ClientProvider     `embed:""`
}

func (l *listUsersCmd) Run(gh *github.Client, s3 *s3.Client) error {
    ...
}

Alternatives

It is possible to do the same today if the *Provider struct above
is actually a Go embed instead of a Kong embed, and it is exported.

type GitHubClientProvider struct{ ... }

type listUsersCmd struct {
    GithubClientProvider
    S3ClientProvider
}

The difference is whether the struct defining the flags
is required to be exported or not.

Relates to 840220c (alecthomas#90)

This change adds support for hooks to be called on fields
that are tagged with `embed:""`.

### Use case

If a command has several subcommands,
many (but not all) of which need the same external resource,
this allows defining the flag-level inputs for that resource centrally,
and then using `embed:""` in any command that needs that resource.

For example, imagine:

```go
type githubClientProvider struct {
    Token string `name:"github-token" env:"GITHUB_TOKEN"`
    URL   string `name:"github-url" env:"GITHUB_URL"`
}

func (g *githubClientProvider) BeforeApply(kctx *kong.Context) error {
  return kctx.BindToProvider(func() (*github.Client, error) {
    return github.NewClient(...), nil
  })
}
```

Then, any command that needs GitHub client will add this field,
any other resource providers it needs,
and add parameters to its `Run` method to accept those resources:

```go
type listUsersCmd struct {
    GitHub githubClientProvider `embed:""`
    S3     s3ClientProvider     `embed:""`
}

func (l *listUsersCmd) Run(gh *github.Client, s3 *s3.Client) error {
    ...
}
```

### Alternatives

It is possible to do the same today if the `*Provider` struct above
is actually a Go embed instead of a Kong embed, *and* it is exported.

```
type GitHubClientProvider struct{ ... }

type listUsersCmd struct {
    GithubClientProvider
    S3ClientProvider
}
```

The difference is whether the struct defining the flags
is required to be exported or not.
Copy link
Owner

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Nice, this is awesome, thanks!

@alecthomas alecthomas merged commit 9c08a58 into alecthomas:master Jan 29, 2025
5 checks passed
abhinav added a commit to abhinav/kong that referenced this pull request Jan 29, 2025
Follow up to alecthomas#493 and 840220c

Kong currently supports hooks on embedded fields of a parsed node,
but only at the first level of embedding:

```
type mainCmd struct {
    FooOptions
}

type FooOptions struct {
    BarOptions
}

func (f *FooOptions) BeforeApply() error {
    // this will be called
}

type BarOptions struct {
}

func (b *BarOptions) BeforeApply() error {
    // this will not be called
}
```

This change adds support for hooks to be defined
on embedded fields of embedded fields so that the above
example would work as expected.

Per alecthomas#493, the definition of "embedded" field is adjusted to mean:

- Any anonymous (Go-embedded) field that is exported
- Any non-anonymous field that is tagged with `embed:""`

*Testing*:
Includes a test case for embedding an anonymous field in an `embed:""`
and an `embed:""` field in an anonymous field.
alecthomas pushed a commit that referenced this pull request Jan 30, 2025
* hooks: Recursively search embedded fields for methods

Follow up to #493 and 840220c

Kong currently supports hooks on embedded fields of a parsed node,
but only at the first level of embedding:

```
type mainCmd struct {
    FooOptions
}

type FooOptions struct {
    BarOptions
}

func (f *FooOptions) BeforeApply() error {
    // this will be called
}

type BarOptions struct {
}

func (b *BarOptions) BeforeApply() error {
    // this will not be called
}
```

This change adds support for hooks to be defined
on embedded fields of embedded fields so that the above
example would work as expected.

Per #493, the definition of "embedded" field is adjusted to mean:

- Any anonymous (Go-embedded) field that is exported
- Any non-anonymous field that is tagged with `embed:""`

*Testing*:
Includes a test case for embedding an anonymous field in an `embed:""`
and an `embed:""` field in an anonymous field.

* Use recursion to build up the list of receivers

The 'receivers' parameter helps avoid constant memory allocation
as the backing storage for the slice is reused across recursive calls.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants