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

feat: support complex flag relationships #468

Merged
merged 12 commits into from Aug 24, 2022
2 changes: 2 additions & 0 deletions src/config/config.ts
Expand Up @@ -751,6 +751,7 @@ export async function toCached(c: Command.Class, plugin?: IPlugin): Promise<Comm
helpGroup: flag.helpGroup,
allowNo: flag.allowNo,
dependsOn: flag.dependsOn,
relationships: flag.relationships,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would we want to use union types to make it not possible to use both relationships and any of the simple ones (dependsOn, exclusive, exactlyOne) so that it doesn't get really nasty?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel comfortable not putting any guard rails up for that. People can make a mess of their flags if they want

For the sf side, we could always add some linting rules to guide people away from doing too much mixing and matching

exclusive: flag.exclusive,
}
} else {
Expand All @@ -768,6 +769,7 @@ export async function toCached(c: Command.Class, plugin?: IPlugin): Promise<Comm
multiple: flag.multiple,
options: flag.options,
dependsOn: flag.dependsOn,
relationships: flag.relationships,
exclusive: flag.exclusive,
default: await defaultToCached(flag),
}
Expand Down
10 changes: 10 additions & 0 deletions src/interfaces/parser.ts
Expand Up @@ -112,6 +112,16 @@ export type FlagProps = {
hidden?: boolean;
required?: boolean;
dependsOn?: string[];
relationships?: {
dependsOn?: {
type: 'all' | 'atLeastOne';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

none could allow exclusive to go away?

flags: string[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flags: string[] | Array<{flag: string, value: any}> would allow you to say things like:

flag A can't be used if flag B has value X (exclusive)
or
you can't use flag A unless flag B has value Y (dependsOn)

or even (allow or disallow multiple values from a flag)

[{ flag: 'a', value: 'x'}, {flag: 'a', value: 'y'}]

};
exclusive?: {
type: 'all' | 'atLeastOne';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checking my understanding:

'all' => it's an error if you use all the flags listed?
'atLeastOne' => it's an error if you use any of the flags listed?

flags: string[];
}
};
exclusive?: string[];
}

Expand Down
124 changes: 92 additions & 32 deletions src/parser/validate.ts
Expand Up @@ -6,7 +6,7 @@ import {
RequiredFlagError,
UnexpectedArgsError,
} from './errors'
import {ParserArg, ParserInput, ParserOutput, Flag} from '../interfaces'
import {ParserArg, ParserInput, ParserOutput, Flag, CompletableFlag} from '../interfaces'

export function validate(parse: {
input: ParserInput;
Expand Down Expand Up @@ -57,44 +57,104 @@ export function validate(parse: {
function validateFlags() {
for (const [name, flag] of Object.entries(parse.input.flags)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to see if parallelizing this would help with perf (both the various validations for each flag, AND doing all the flags together)

if (parse.output.flags[name] !== undefined) {
for (const also of flag.dependsOn || []) {
if (!parse.output.flags[also]) {
throw new CLIError(
`--${also}= must also be provided when using --${name}=`,
)
validateRelationships(name, flag)
validateDependsOn(name, flag.dependsOn ?? [])
validateExclusive(name, flag.exclusive ?? [])
validateExactlyOne(name, flag.exactlyOne ?? [])
} else if (flag.required) {
throw new RequiredFlagError({parse, flag})
} else if (flag.exactlyOne && flag.exactlyOne.length > 0) {
validateAcrossFlags(flag)
}
}
}

function validateExclusive(name: string, exclusive: string[]) {
for (const also of exclusive) {
// do not enforce exclusivity for flags that were defaulted
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider making this configurable. Flag B exists, but can't be used unless you make Flag A something other than its default

if (
parse.output.metadata.flags[also] &&
parse.output.metadata.flags[also].setFromDefault
)
continue
if (
parse.output.metadata.flags[name] &&
parse.output.metadata.flags[name].setFromDefault
)
continue
if (parse.output.flags[also]) {
throw new CLIError(
`--${also}= cannot also be provided when using --${name}=`,
)
}
}
}

function validateExactlyOne(name: string, exactlyOne: string[]) {
for (const also of exactlyOne || []) {
if (also !== name && parse.output.flags[also]) {
throw new CLIError(
`--${also}= cannot also be provided when using --${name}=`,
)
}
}
}

function validateDependsOn(name: string, dependsOn: string[]) {
for (const also of dependsOn || []) {
if (!parse.output.flags[also]) {
throw new CLIError(
`--${also}= must also be provided when using --${name}=`,
)
}
}
}

function validateRelationships(name: string, flag: CompletableFlag<any>) {
if (!flag.relationships) return

if (flag.relationships?.dependsOn) {
const dependsOnFlags = flag.relationships.dependsOn.flags ?? []
if (flag.relationships?.dependsOn.type === 'all') {
validateDependsOn(name, dependsOnFlags)
}

if (flag.relationships.dependsOn.type === 'atLeastOne') {
let foundAtLeastOne = false
for (const flag of dependsOnFlags) {
if (parse.output.flags[flag]) {
foundAtLeastOne = true
break
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let foundAtLeastOne = false
for (const flag of dependsOnFlags) {
if (parse.output.flags[flag]) {
foundAtLeastOne = true
break
const foundAtLeastOne = dependsOnFlags.some((flag) => parse.output.flags[flag])

}
}

for (const also of flag.exclusive || []) {
// do not enforce exclusivity for flags that were defaulted
if (
parse.output.metadata.flags[also] &&
parse.output.metadata.flags[also].setFromDefault
)
continue
if (
parse.output.metadata.flags[name] &&
parse.output.metadata.flags[name].setFromDefault
)
continue
if (parse.output.flags[also]) {
throw new CLIError(
`--${also}= cannot also be provided when using --${name}=`,
)
}
if (!foundAtLeastOne) {
const flags = (dependsOnFlags ?? []).map(f => `--${f}`).join(', ')
throw new CLIError(`One of the following must be provided when using --${name}: ${flags}`)
}
}
}

if (flag.relationships?.exclusive) {
const exclusiveFlags = flag.relationships.exclusive.flags ?? []

for (const also of flag.exactlyOne || []) {
if (also !== name && parse.output.flags[also]) {
throw new CLIError(
`--${also}= cannot also be provided when using --${name}=`,
)
if (flag.relationships.exclusive.type === 'all') {
validateExclusive(name, exclusiveFlags)
}

if (flag.relationships.exclusive.type === 'atLeastOne') {
let foundAtLeastOne = false
for (const flag of exclusiveFlags) {
if (parse.output.flags[flag]) {
foundAtLeastOne = true
break
}
}
} else if (flag.required) {
throw new RequiredFlagError({parse, flag})
} else if (flag.exactlyOne && flag.exactlyOne.length > 0) {
validateAcrossFlags(flag)

if (!foundAtLeastOne) {
const flags = (exclusiveFlags ?? []).map(f => `--${f}`).join(', ')
throw new CLIError(`The following cannot be provided when using --${name}: ${flags}`)
}
}
}
}
Expand Down