feat: add function to override default envvar name

[CarlosRDomin]

Addresses #293

[alexflint]

For now, no need to export any of the new types or functions in this file. Could the new config field just have the type func(field reflect.StructField) string directly inlined within the struct?

[CarlosRDomin]

Yup, no problem, I'm generally a fan of DRY but if you prefer to repeat it explicitly that's fine

[alexflint]

The down side of having this function be part of the library is that once it's in, it can't be changed because people will start to rely on it. For now, would it be alright to leave this out?

[CarlosRDomin]

Yeah I can push it to my own repo. Out of curiosity, what would be the downside of leaving this implementation in the library? It's still opt-in if anyone wants to rely on it, and is covered with tests that document the expected behavior

[alexflint]

I'd say leave this out and just directly use strings.ToUpper as needed in the main code in parser.go. Will leave separate comment indicating what I mean.

[alexflint]

Just to keep things as straightforward as possible, could this chunk of code be

if defaultEnvName == nil {
   spec.env = envPrefix + strings.ToUpper(field.Name)
} else {
  spec.env = envPrefix + defaultEnvName(field)
}

[CarlosRDomin]

Pushed all the changes, let me know what you think

[CarlosRDomin]

Hey @alexflint I've addressed your feedback but I've pushed a 3rd commit that I think makes this feature more powerful:

• It calls DefaultEnvName on all fields, so a user could e.g. provide default env var names for all fields without requiring a arg:"env" tag everywhere (returning an empty string still leaves the field unsettable via envvar, similar to leaving DefaultEnvName unset).
  If you like this approach, we could also allow (in a separate PR if needed) to apply the same idea to the cli name, so the config is as DRY as possible.
• It takes envPrefix as an input, so the user has flexibility on how/when to add the prefix (though we can easily rewrite this so the function only takes field reflect.StructField as an input and the library adds the prefix if the function returns a non-empty string)

Let me know if you like it, otherwise, feel free to review up to the 2nd commit and I'll drop/revert this last commit so we can merge (I'm hopeful that the 2nd commit matches your expectations based on the previous feedback). Thanks again for your time and review!

[alexflint]

Hey @CarlosRDomin thanks for these updates. How about adding a second config variable "AllHaveEnv bool" that makes it the case that all fields have environment variables? The reason I suggest this rather than allowing empty return value from DefaultEnvName is that I think the latter would have to be implemented in most cases with an explicit list of field names to include/exclude, meaning that you'd run back into the issue of having a list of string literals somewhere that have to be kept up to date with the names of the fields on the struct, as in:

func myDefaultEnvName(f reflect.StructField) string {
  if f.Name == "abc" || f.Name == "xyz" || ... {
    return ""
  }
  return snakeCaseFromFieldName(f.Name)
}

In my own code, I'd make use of this DefaultEnvName feature, but without turning it on by default for all arguments.

If you're up for implementing AllHaveEnv then I'd be happy to review it as part of this PR.

[CarlosRDomin]

Thanks for the feedback @alexflint! I thought about something similar to AllHaveEnv but initially thought that it would be hard to handle the (perhaps rare) case in which someone wants all but one/some fields to have env (e.g. a password that can only be passed as cli). So then I thought of adding support for a noEnv tag, but then users would have to parse the arg tag in the field.Tag and also we'd have to handle corner cases like setting both env and noEnv, etc so I discarded the idea. The other option is to handle "env:-" to disable env.

Anyway, I think AllHaveEnv is great for my use cases (anything that can be set via cli should be settable via env vars for easy support of deployment via kubernetes) and I'd be more than happy to implement it, let me give it a quick try.

[CarlosRDomin]

Btw would you want me to re-introduce the snakeCaseFromFieldName implementation in an experimental package (e.g. go-arg/exp) so others can reuse it without formally making it part of the library yet? (I'm fine either way)

[CarlosRDomin]

@alexflint pushed. Let me know what you think of this new version, and whether you'd like me to include snakeCaseFromFieldName in an experimental package :)

[alexflint]

Hey this looks great thanks @CarlosRDomin