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

MustProcess: panic if processing fails, return the value #114

Closed
wants to merge 1 commit into from

Conversation

imjasonh
Copy link
Contributor

@imjasonh imjasonh commented Jul 3, 2024

I originally made this PR to kelseyhightower/envconfig#213 but this looks to be the supported alternative

This adds a generic MustProcess method, that panics if processing fails, and returns the spec value.

Example usage:

package main

import (
  "log"

  "github.com/sethvargo/go-envconfig"
)

var env = envconfig.MustProcess(context.Background(), &struct{
  Foo string `env:"FOO"`
}{})

func main() {
  log.Println("your FOO is " + env.Foo)
}

This lets the env struct be moved to an init-time var, out of func main, and retains all the env parsing behavior we've all come to know and love.

The structure of that var statement is kind of gross (inline-defining a struct and its empty value, and passing it to the method), if I'm missing some way to make it better, let me know.

Type parameters were required, since MustProcess(ctx, interface{}) interface{} loses information about the input type. You could cast back to the type you want, but generics are better IMO, since it lets you do it all in one statement.

Signed-off-by: Jason Hall <jason@chainguard.dev>
@sethvargo
Copy link
Owner

I think this is achievable many idiomatic ways today, so I'm not sold on adding another API function. You could use an init function or a func enclosure that returns the var. I also think this is generally a bad practice because (a) you should have a concrete config type and (b) it makes testing a nightmare.

What problem are you trying to solve by not having the parsing in main?

@imjasonh
Copy link
Contributor Author

imjasonh commented Jul 5, 2024

I think this is achievable many idiomatic ways today, so I'm not sold on adding another API function. You could use an init function or a func enclosure that returns the var. I also think this is generally a bad practice because (a) you should have a concrete config type and (b) it makes testing a nightmare.

What problem are you trying to solve by not having the parsing in main?

These are all perfectly reasonable questions. If this isn't accepted, I totally understand. 😄

This came about from looking at how we've used envconfig (Kelsey's version), which generally follows this form:

package main

type env struct {
  Port        int    `envconfig:"port", required:"true"`
  BackendAddr string `envconfig:"backend", required:"true"`
  ... etc.
}

func main() {
  var env env
  if err := envconfig.Process("", &env); err != nil {
    log.Fatalf("bad env: %v", err)
  }
  ... use env.BackendAddr, etc.
}

This is ...fine. Some examples define type env struct inside func main(), a couple do it in func init to get it out of func main(), there's maybe half a dozen similar ways to do it. I do realize I'm proposing another one. 😆

In any case, our usage tends to be only inside package main, where we generally don't have a need to test the type. It's generally a shortcut for os.Getenv and verbose manual error-prone validation. The env values usually go straight in to a client setup and the client gets passed to a type that does the real work, and gets tested that way. The type is usually concretely defined today, but is unexported from package main so it doesn't really amount to much.

My line of thinking was roughly:

  1. we always log.Fatal when Process returns its err; why not just have MustProcess (Kelsey's has this, but it doesn't return the value) -- this saves 2 lines of boilerplate.
  2. when we have MustProcess, it could return the value -- this saves another line of boilerplate.
  3. when we have a one-liner, it could go into the package-level var definition, with some anonymous struct contortions -- this lets us move it out of func main().

That line of thinking might be flawed, or overly clever, but that's how I got to this approach.

If this isn't considered an idiomatic way to use the library, I'll accept that. Thanks for this library, and for your thoughtful response 😄

@sethvargo
Copy link
Owner

@imjasonh I cleaned this up and updated docs in #115

@imjasonh
Copy link
Contributor Author

imjasonh commented Jul 7, 2024

@imjasonh I cleaned this up and updated docs in #115

Thanks! 😍

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.

3 participants