-
-
Notifications
You must be signed in to change notification settings - Fork 66
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 env option to option method #547
Comments
From reading this issue, I just noticed this line in the docs
That's super useful! How would this interact with dot values like I'm wondering if it makes sense to support any name (rather than a prefix) in the In terms of API feedback, I find it a bit counter-intuitive that the |
Thank you for your thoughts @chmac!
That's a good question. I haven't thought about dotted env vars yet, but I think that could be a requirement for this feature. I think your suggestion might make sense. But an env var defined as
I think it would also be fine for me to also allow defining the name. The prefix could also default to the main command name or to a prefix defined on the main command with a
I'm not sure at the moment if it makes sense to support more env var options than the prefix or a name option. I think if env var and option are different, it is better to define them separately, because then you would probably need different descriptions and/or hints as well.
I agree it could be confusing, but i think i would prefere to put it in an object like this then Maybe something like this could work: new Command()
.name("deno") // -> deno is the default prefix
// .prefix("foo") // -> override default prefix
// current implementation:
.env(
"DENO_INSTALL_ROOT=<path:string>", // -> DENO_INSTALL_ROOT
"Set install root.",
{ prefix: "DENO_" }, // -> opts.installRoot
)
// new implementation:
.env(
"DENO_INSTALL_ROOT=<path:string>", // -> DENO_INSTALL_ROOT
"Set install root.",
{ prefix: true }, // -> opts.installRoot (prefix defaults to main command name or prefix)
)
.option(
"--install-root <path:string>",
"Set install root.",
{ env: true } // -> INSTALL_ROOT or DENO_INSTALL_ROOT (in this case, prefix defaults to main command name or prefix)
)
.option(
"--install-root <path:string>",
"Set install root.",
{ env: "DENO_INSTALL_ROOT" } // -> DENO_INSTALL_ROOT
)
.option(
"--install-root <path:string>",
"Set install root.",
{ env: { prefix: "DENO_" } } // -> DENO_INSTALL_ROOT
)
.option(
"--install-root <path:string>",
"Set install root.",
{ env: { prefix: true } } // -> DENO_INSTALL_ROOT (prefix defaults to main command name or prefix)
) But I also thought about automatically detecting the environment variable prefix by checking if the environment variable name starts with |
A couple of thoughts come up for me on prefixes.
Hmm, perhaps the answer to the second is yes. For example I have a script that reads the value of It seems to me like the prefix serves 2 purposes. First, it means my environment variables are unique, and so less likely to clash with other scripts already installed. Second, it makes them easier to access on my I like the idea of being able to set a global env prefix, maybe In the case of an option, presumably the env var could be called anything, and the value will end up on whatever the option is called. Therefore in that context, the .option(
"--install-root <path:string>",
"Set install root.",
{ env: "DENO_INSTALL_ROOT" } // -> DENO_INSTALL_ROOT
) With the caveat that this would be equivalent: .envPrefix('DENO_')
.option(
"--install-root <path:string>",
"Set install root.",
{ env: "INSTALL_ROOT" } // -> DENO_INSTALL_ROOT
) I also liked your idea of allowing Sorry if this is a bit rambling, I'm half thinking out loud! |
To reduce verbosity, the option method should have an env option to auto register an environment variable for this option.
Without prefix:
With prefix:
Maybe add also a
.prefix()
method to automatically prefix all env vars.The text was updated successfully, but these errors were encountered: