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
1 change: 1 addition & 0 deletions src/config/config.ts
Expand Up @@ -768,6 +768,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
7 changes: 7 additions & 0 deletions src/interfaces/parser.ts
Expand Up @@ -86,6 +86,12 @@ export type DefaultContext<T> = {
export type Default<T> = T | ((context: DefaultContext<T>) => Promise<T>)
export type DefaultHelp<T> = T | ((context: DefaultContext<T>) => Promise<string | undefined>)

export type FlagRelationship = string | {name: string; when: (flags: Record<string, unknown>) => Promise<boolean>};
export type Relationship = {
type: 'all' | 'some' | 'none';
Copy link
Member

Choose a reason for hiding this comment

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

consider one or 1 since we've already got an exactlyOne since it happened often enough.

flags: FlagRelationship[];
}

export type FlagProps = {
name: string;
char?: AlphabetLowercase | AlphabetUppercase;
Expand All @@ -112,6 +118,7 @@ export type FlagProps = {
hidden?: boolean;
required?: boolean;
dependsOn?: string[];
relationships?: Relationship[];
exclusive?: string[];
}

Expand Down
2 changes: 1 addition & 1 deletion src/parser/index.ts
Expand Up @@ -27,7 +27,7 @@ export async function parse<TFlags, GFlags, TArgs extends { [name: string]: stri
}
const parser = new Parser(input)
const output = await parser.parse()
m.validate({input, output})
await m.validate({input, output})
return output as ParserOutput<TFlags, GFlags, TArgs>
}

Expand Down
138 changes: 100 additions & 38 deletions src/parser/validate.ts
@@ -1,3 +1,4 @@
/* eslint-disable no-await-in-loop */
import {CLIError} from '../errors'

import {
Expand All @@ -6,9 +7,10 @@ import {
RequiredFlagError,
UnexpectedArgsError,
} from './errors'
import {ParserArg, ParserInput, ParserOutput, Flag} from '../interfaces'
import {ParserArg, ParserInput, ParserOutput, Flag, CompletableFlag} from '../interfaces'
import {FlagRelationship} from '../interfaces/parser'

export function validate(parse: {
export async function validate(parse: {
input: ParserInput;
output: ParserOutput;
}) {
Expand Down Expand Up @@ -54,43 +56,13 @@ export function validate(parse: {
}
}

function validateFlags() {
async 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}=`,
)
}
}

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}=`,
)
}
}

for (const also of flag.exactlyOne || []) {
if (also !== name && parse.output.flags[also]) {
throw new CLIError(
`--${also}= cannot also be provided when using --${name}=`,
)
}
}
await validateRelationships(name, flag)
await validateDependsOn(name, flag.dependsOn ?? [])
await 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) {
Expand All @@ -99,6 +71,96 @@ export function validate(parse: {
}
}

async function resolveFlags(flags: FlagRelationship[]): Promise<Record<string, unknown>> {
const promises = flags.map(async flag => {
if (typeof flag === 'string') {
return [flag, parse.output.flags[flag]]
}

const result = await flag.when(parse.output.flags)
return result ? [flag.name, parse.output.flags[flag.name]] : null
})
const resolved = await Promise.all(promises)
return Object.fromEntries(resolved.filter(r => r !== null) as [string, unknown][])
}

async function validateExclusive(name: string, flags: FlagRelationship[]) {
const resolved = await resolveFlags(flags)
const keys = Object.keys(resolved).reduce((acc, key) => {
if (resolved[key]) acc.push(key)
return acc
}, [] as string[])
Copy link
Member

Choose a reason for hiding this comment

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

not sure why this reduce needs to be here. Another design would be

Object.entries(resolved).forEach(

and use 1 && conditional to check what you're checking to get to an error

  1. the value is truthy AND
    2.parse.output.metadata.flags[flag]?.setFromDefault is falsy AND
  2. parse.output.metadata.flags[name]?.setFromDefault is falsy AND
  3. parse.output.flags[flag] is truthy
    --> throw!

that way it's only 1 iteration, the conditionals can be in whatever order is most likely to resolve soonest and it'll fail on the first error


for (const flag of keys) {
// 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[flag] &&
parse.output.metadata.flags[flag].setFromDefault
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
parse.output.metadata.flags[flag] &&
parse.output.metadata.flags[flag].setFromDefault
parse.output.metadata.flags[flag]?.setFromDefault

etc on the others.

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

function validateExactlyOne(name: string, exactlyOne: FlagRelationship[]) {
for (const flag of exactlyOne || []) {
Copy link
Member

Choose a reason for hiding this comment

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

defaults in the signature

Suggested change
function validateExactlyOne(name: string, exactlyOne: FlagRelationship[]) {
for (const flag of exactlyOne || []) {
function validateExactlyOne(name: string, exactlyOne: FlagRelationship[] = []) {
for (const flag of exactlyOne) {

const flagName = typeof flag === 'string' ? flag : flag.name
if (flagName !== name && parse.output.flags[flagName]) {
throw new CLIError(
`--${flagName} cannot also be provided when using --${name}`,
)
}
}
}

async function validateDependsOn(name: string, flags: FlagRelationship[]) {
const resolved = await resolveFlags(flags)
const foundAll = Object.values(resolved).every(Boolean)
if (!foundAll) {
const formattedFlags = Object.keys(resolved).map(f => `--${f}`).join(', ')
throw new CLIError(
`All of the following must be provided when using --${name}: ${formattedFlags}`,
)
}
}

async function validateSome(name: string, flags: FlagRelationship[]) {
const resolved = await resolveFlags(flags)
const foundAtLeastOne = Object.values(resolved).some(Boolean)
if (!foundAtLeastOne) {
const formattedFlags = Object.keys(resolved).map(f => `--${f}`).join(', ')
throw new CLIError(`One of the following must be provided when using --${name}: ${formattedFlags}`)
}
}

async function validateRelationships(name: string, flag: CompletableFlag<any>) {
if (!flag.relationships) return
for (const relationship of 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.

worth parallelizing?

Copy link
Member

Choose a reason for hiding this comment

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

ux idea: as written, you'll get a failure on the first validation to throw. For the user, they'll fix one error and then hit the next.

You could run this with Promise.allSettled and maybe display all the errors in the output.

const flags = relationship.flags ?? []

if (relationship.type === 'all') {
await validateDependsOn(name, flags)
}

if (relationship.type === 'some') {
await validateSome(name, flags)
}

if (relationship.type === 'none') {
await validateExclusive(name, flags)
}
}
}

validateArgs()
validateFlags()
await validateFlags()
}
12 changes: 6 additions & 6 deletions test/parser/parse.test.ts
Expand Up @@ -903,7 +903,7 @@ See more help with --help`)
message = error.message
}

expect(message).to.equal('--bar= must also be provided when using --foo=')
expect(message).to.equal('All of the following must be provided when using --foo: --bar')
})
})

Expand Down Expand Up @@ -940,7 +940,7 @@ See more help with --help`)
message = error.message
}

expect(message).to.equal('--bar= cannot also be provided when using --foo=')
expect(message).to.equal('--bar=b cannot also be provided when using --foo')
})
})

Expand Down Expand Up @@ -974,7 +974,7 @@ See more help with --help`)
message = error.message
}

expect(message).to.equal('--bar= cannot also be provided when using --foo=')
expect(message).to.equal('--bar cannot also be provided when using --foo')
})

it('succeeds if exactly one', async () => {
Expand Down Expand Up @@ -1035,7 +1035,7 @@ See more help with --help`)
message = error.message
}

expect(message).to.equal('--else= cannot also be provided when using --foo=')
expect(message).to.equal('--else cannot also be provided when using --foo')
})

it('handles cross-references/pairings that don\'t make sense', async () => {
Expand All @@ -1053,7 +1053,7 @@ See more help with --help`)
message1 = error.message
}

expect(message1).to.equal('--bar= cannot also be provided when using --foo=')
expect(message1).to.equal('--bar cannot also be provided when using --foo')

let message2 = ''
try {
Expand All @@ -1064,7 +1064,7 @@ See more help with --help`)
message2 = error.message
}

expect(message2).to.equal('--else= cannot also be provided when using --bar=')
expect(message2).to.equal('--else cannot also be provided when using --bar')

const out = await parse(['--foo', 'a', '--else', '4'], {
flags: crazyFlags,
Expand Down