oclif / core

Node.js Open CLI Framework. Built by Salesforce.
https://oclif.io
MIT License
208 stars 71 forks source link

side effects of setting flag defaults #854

Open cristiand391 opened 1 year ago

cristiand391 commented 1 year ago

Is your feature request related to a problem? Please describe. not really a feature request, just wanted to post this for others to be aware.

the way oclif handles flag defaults makes it hard to tell wether a user really typed that flag or is just a default value from flag.default: ()=>{}.

Workaround: search for the flag in argv, like this: https://github.com/salesforcecli/plugin-telemetry/blob/6eaedbed2bdbdf2979be9f6bcd9fad5c43ae2a5b/src/commandExecution.ts#L180-L181 so you need to carefully search for all valid flag styles (long/short, with = sep).

Example: in sf we have a requiredHubFlag for commands that interact with Salesforce devhubs that is both required and has a default value handler: https://github.com/salesforcecli/sf-plugins-core/blob/58d8e9a08dda865850adcdec0d791ee0195871f9/src/flags/orgFlags.ts#L175-L189

it seems the parser will first set any flag value that has a flag.default val/func defined and after that validate if flag.required: true that the flag has a value, which at the time this validation happens it's true.

some side-effects of this:

  1. can't determine at if a user really typed a flag or is just a default value.
  2. if a flag has a default and required: true, help text is wrong ((required) when it may not be)

Describe the solution you'd like maybe the parser could set a prop that we could check to see if it was typed or not. Not sure what could we do with the help text.

Describe alternatives you've considered 1) parsing argv 2) set flags with no defaults, then handle them in the run method if they weren't specified.

Additional context We got an issue in our repo about this but some internal people noticed multiple times: https://github.com/forcedotcom/cli/issues/2538

AllanOricil commented 1 year ago

I believe this could be resolved if required could accept a function having in its context:

required: Boolean | Function

Then a fix for this issue would be something like this:

required: (ctx) => !this.defaultValue

Where ctx = { arg, flags } and this is injected (bind) to the given function scope and refers to the flag's own object

where this.defaultValue refers to the flag's resolved default value

We could implement this together because if I try to do it alone it would take a really good amount of time. There are a few things I don't have any clue how to do it.

Maybe this could potentially solve #829 as well. What do u think @bader-nasser

bader-nasser commented 1 year ago

@AllanOricil I like the idea of accepting a function but I don't think this would solve my issue.

I have many optional flags but one of three flags must be provided to make the command useful, but this function seems like it'll set each one of these flags to be required as far as I understand the idea.

My issue maybe could be solved by using a special property in the flags object:

static flags = {
  _required: {
    flags: ['flag-a', 'flag-b', 'flag-c'],
    type: 'one' // or 'all' or 'some' or 'any'
  }
}