feat: support complex flag relationships #468

mdonnalley · 2022-08-10T22:18:29Z

Lay groundwork to support more advanced flag relationship validation

Example usage

export default class MyCommand extends Command {
  static flags = {
    one: Flags.string(),
    two: Flags.string(),
    three: Flags.string(),
    four: Flags.string(),
    five: Flags.string(),
    six: Flags.string(),
    foo: Flags.string({
      relationships: [
         // always require the 'one' flag    
         { type: 'all', flags: ['one']},

         // require either of the 'two' or 'three' flags
         { type: 'some', flags: ['two', 'three']},

         // never allow the 'four' flag
         { type: 'none', flags: ['four']},

         // never allow the 'five' flag but only when it is equal to 'foo'
         { type: 'none', flags: [{name: 'five', when: (flags) => flags.five === 'foo'}]},

         // always require the 'six' flag but only when 'one' is equal to 'foo'
         { type: 'all', flags: [{name: 'six', when: (flags) => flags.one === 'foo'}]},
      ]
    }),
  }
}

Closes oclif/oclif#323

mshanemc · 2022-08-10T22:32:25Z

src/interfaces/parser.ts

@@ -112,6 +112,16 @@ export type FlagProps = {
  hidden?: boolean;
  required?: boolean;
  dependsOn?: string[];
+  relationships?: {
+    dependsOn?: {
+      type: 'all' | 'atLeastOne';


none could allow exclusive to go away?

mshanemc · 2022-08-10T22:37:20Z

src/interfaces/parser.ts

+  relationships?: {
+    dependsOn?: {
+      type: 'all' | 'atLeastOne';
+      flags: string[];


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

mshanemc · 2022-08-10T22:38:27Z

src/parser/validate.ts

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


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

mshanemc · 2022-08-10T22:41:33Z

src/parser/validate.ts

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


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])

mshanemc · 2022-08-10T22:44:39Z

src/interfaces/parser.ts

+      flags: string[];
+    };
+    exclusive?: {
+      type: 'all' | 'atLeastOne';


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?

mshanemc · 2022-08-10T22:46:35Z

src/config/config.ts

@@ -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,


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?

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

mshanemc · 2022-08-18T00:52:36Z

src/interfaces/parser.ts

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


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

mshanemc · 2022-08-18T00:58:03Z

src/parser/validate.ts

+
+  async function validateRelationships(name: string, flag: CompletableFlag<any>) {
+    if (!flag.relationships) return
+    for (const relationship of flag.relationships) {


worth parallelizing?

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.

mshanemc

approved with some ideas/suggestions

mshanemc · 2022-08-18T01:01:28Z

test/parser/validate.test.ts

+      })
+    })
+
+    describe('type: some', () => {


testing ideas:

it looks like you've always got length = 1 for relationships. I'd be interested in maybe a "big test" that

tests an array of several relationships

includes relationships of different types

mshanemc · 2022-08-18T01:02:17Z

test/parser/validate.test.ts

+                {
+                  type: 'all',
+                  flags: [
+                    'cookies',


mshanemc · 2022-08-18T12:17:52Z

src/parser/validate.ts

+        parse.output.metadata.flags[flag] &&
+        parse.output.metadata.flags[flag].setFromDefault


Suggested change

parse.output.metadata.flags[flag] &&

parse.output.metadata.flags[flag].setFromDefault

parse.output.metadata.flags[flag]?.setFromDefault

etc on the others.

mshanemc · 2022-08-18T12:19:44Z

src/parser/validate.ts

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


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) {

mshanemc · 2022-08-18T12:29:09Z

src/parser/validate.ts

+
+  async function validateRelationships(name: string, flag: CompletableFlag<any>) {
+    if (!flag.relationships) return
+    for (const relationship of flag.relationships) {


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.

mshanemc · 2022-08-18T12:32:14Z

src/parser/validate.ts

@@ -54,43 +56,13 @@ export function validate(parse: {
    }
  }

-  function validateFlags() {
+  async function validateFlags() {
    for (const [name, flag] of Object.entries(parse.input.flags)) {


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)

mshanemc · 2022-08-18T12:43:17Z

src/parser/validate.ts

+    const keys = Object.keys(resolved).reduce((acc, key) => {
+      if (resolved[key]) acc.push(key)
+      return acc
+    }, [] as string[])


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

the value is truthy AND
2.parse.output.metadata.flags[flag]?.setFromDefault is falsy AND

parse.output.metadata.flags[name]?.setFromDefault is falsy AND

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

feat: support complex flag relationships

ace31f3

mdonnalley self-assigned this Aug 10, 2022

mdonnalley requested a review from mshanemc August 10, 2022 22:18

mshanemc reviewed Aug 10, 2022

View reviewed changes

feat: change type

0fe00cb

mshanemc reviewed Aug 10, 2022

View reviewed changes

mdonnalley added 2 commits August 10, 2022 20:13

feat: add when property

f96df37

fix: more improvements

bbf8c9d

mdonnalley requested a review from mshanemc August 11, 2022 05:31

mdonnalley added 6 commits August 11, 2022 09:08

feat: update when signature

3c58582

chore: tests

c67882a

chore: more tests

4deb315

Merge branch 'main' into mdonnalley/flag-relationships

c966aaf

Merge branch 'main' into mdonnalley/flag-relationships

8553952

chore: rename never to none

57c1c97

mshanemc reviewed Aug 18, 2022

View reviewed changes

mshanemc previously approved these changes Aug 18, 2022

View reviewed changes

Merge branch 'main' into mdonnalley/flag-relationships

1e2aceb

mdonnalley dismissed mshanemc’s stale review via 1e2aceb August 23, 2022 20:53

chore: code review

c455eda

mdonnalley merged commit 222d1f6 into main Aug 24, 2022

mdonnalley deleted the mdonnalley/flag-relationships branch August 24, 2022 15:30

Amndeep7 mentioned this pull request Sep 29, 2022

Update flag relationships to use new oclif functionality mitre/saf#710

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support complex flag relationships #468

feat: support complex flag relationships #468

mdonnalley commented Aug 10, 2022 •

edited

mshanemc Aug 10, 2022

mshanemc Aug 10, 2022

mshanemc Aug 10, 2022

mshanemc Aug 10, 2022

mshanemc Aug 10, 2022

mshanemc Aug 10, 2022

mdonnalley Aug 11, 2022

mshanemc Aug 18, 2022

mshanemc Aug 18, 2022

mshanemc Aug 18, 2022

mshanemc left a comment

mshanemc Aug 18, 2022

mshanemc Aug 18, 2022

mshanemc Aug 18, 2022

mshanemc Aug 18, 2022

mshanemc Aug 18, 2022

mshanemc Aug 18, 2022

mshanemc Aug 18, 2022

		parse.output.metadata.flags[flag] &&
		parse.output.metadata.flags[flag].setFromDefault

	parse.output.metadata.flags[flag] &&
	parse.output.metadata.flags[flag].setFromDefault
	parse.output.metadata.flags[flag]?.setFromDefault

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

feat: support complex flag relationships #468

feat: support complex flag relationships #468

Conversation

mdonnalley commented Aug 10, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshanemc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdonnalley commented Aug 10, 2022 •

edited