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 env option to option method #547

Open
c4spar opened this issue Feb 21, 2023 · 3 comments
Open

Add env option to option method #547

c4spar opened this issue Feb 21, 2023 · 3 comments
Labels
enhancement New feature or request module:command command module

Comments

@c4spar
Copy link
Owner

c4spar commented Feb 21, 2023

To reduce verbosity, the option method should have an env option to auto register an environment variable for this option.

Without prefix:

new Command()
  .option("-f, --foo", "example description.")
  .env("FOO", "example description.")
new Command()
  .option("-f, --foo", "example description.", { env: true })

With prefix:

new Command()
  .option("-f, --foo", "example description.")
  .env("PREFIX_FOO", "example description.")
new Command()
  .option("-f, --foo", "example description.", { env:  "PREFIX_" })

Maybe add also a .prefix() method to automatically prefix all env vars.

@c4spar c4spar added enhancement New feature or request module:command command module labels Feb 21, 2023
@chmac
Copy link

chmac commented Feb 24, 2023

From reading this issue, I just noticed this line in the docs

If an option with the same name is defined, the option will override the environment variable.

That's super useful!

How would this interact with dot values like .option("-f.b --foo-bar <foo:string>")? Would the equivalent env var be FOO.BAR?

I'm wondering if it makes sense to support any name (rather than a prefix) in the {env: ....} option? But perhaps this is unnecessary. I'm trying to imagine a scenario where my option might be called --foo but my environment variable might be called `BAR="something".

In terms of API feedback, I find it a bit counter-intuitive that the env can either be true or a prefix. I'd assume that the prefix would be a separate option, like env_prefix. But I can also see the elegance of just having a single property to determine whether this option also has a matching environment variable or not.

@c4spar
Copy link
Owner Author

c4spar commented Feb 24, 2023

Thank you for your thoughts @chmac!

How would this interact with dot values like .option("-f.b --foo-bar foo:string")? Would the equivalent env var be FOO.BAR?

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 FOO.BAR would still have to be called as FOO_BAR and also be displayed that way in the help, since dots are not allowed in variable names.

I'm wondering if it makes sense to support any name (rather than a prefix) in the {env: ....} option? But perhaps this is unnecessary.

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 .prefix() or .envVarPrefix() method.

I'm trying to imagine a scenario where my option might be called --foo but my environment variable might be called `BAR="something".

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.

In terms of API feedback, I find it a bit counter-intuitive that the env can either be true or a prefix. I'd assume that the prefix would be a separate option, like env_prefix.

I agree it could be confusing, but i think i would prefere to put it in an object like this then { env: { prefix: "FOO_" } }.


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 ${mainCommandName.toUpperCase()}_ or ${mainCommandPrefix}_, so there is no need to define a prefix if the prefix matches the cli name. Also { prefix: true } would not be necessary.

@chmac
Copy link

chmac commented Feb 24, 2023

A couple of thoughts come up for me on prefixes.

  • Is the prefix obligatory?
  • Does anyone want to set prefixes on just 1 env var?

Hmm, perhaps the answer to the second is yes. For example I have a script that reads the value of HOME, but I might also want to prefix my own custom env vars.

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 options parameter because they're shorter. The second part seems nice, but in the "nice to have" category rather than the "essential features" category.

I like the idea of being able to set a global env prefix, maybe .envPrefix('FOO_'). In my mind, if I set that, it would apply to all env vars that are registered after it. Of course I could override it on a single var with prefix: ''.

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 prefix is less important. Personally, I like this option:

  .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 env: true which would automatically generate the prefix name, but that could also lead to more confusion. Here's a thought, without the option to set env: true, there's no need to think about how to handle dotted variables. The variable name on options is taken from the option name, and then env var name can be anything. Then I could say --install.root <path:string> and make the env var INSTALL_PATH if I wanted to (which obviously would be a bit silly, but it would work).

Sorry if this is a bit rambling, I'm half thinking out loud!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request module:command command module
Projects
None yet
Development

No branches or pull requests

2 participants