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

side effects of setting flag defaults #854

Open
cristiand391 opened this issue Nov 6, 2023 · 2 comments
Open

side effects of setting flag defaults #854

cristiand391 opened this issue Nov 6, 2023 · 2 comments
Labels
announcement Pinned issues for known bugs or general communication with the community.

Comments

@cristiand391
Copy link
Member

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:
forcedotcom/cli#2538

@AllanOricil
Copy link
Contributor

AllanOricil commented Nov 12, 2023

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

  • parsed flags in case required depends on other flags contex
  • parsed arg in case the flag does not need to be specified when arg is given
  • flag's own 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

  • when defaultValue is falsy, required = true, otherwise required = false

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
Copy link

@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'
  }
}

@mdonnalley mdonnalley added the announcement Pinned issues for known bugs or general communication with the community. label Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
announcement Pinned issues for known bugs or general communication with the community.
Projects
None yet
Development

No branches or pull requests

4 participants