Enhancement: Extend `OptionSets` so they can validate optional and dependant sets as well

[martinlingstuyl]

Some time ago OptionSet was introduced. A way to validate two required options that are mutually exclusive. Some work is currently being done on #3218 to use OptionSets in all relevant commands.

OptionSet currently only validates required options. However, there are quite some commands in which there are mutually exclusive options that are not required.

For example, the command planner task get has the options bucketId and bucketName that are mutually exclusive, but depending on another option. This also applies to planId and planName and ownerGroupId and ownerGroupName.

We also have the command planner task set and planner task add where the options assignedToUserIds and assignedToUserNames are optional and mutually exclusive.

Currently, quite some validation logic needs to be written again and again, while this can be validated easier using something like OptionSet.

Requirements

OptionSets should be able to validate the following scenario's:

1. Required sets of mutually exclusive options
2. Optional sets of mutually exclusive options
3. Sets of mutually exclusive options that are required when a condition applies. (Eg. another option is filled)

Implementation

After discussing it, we decided on the following implementation which takes care of all 3 requirements:

public optionSets: OptionSet[]  {
  return [ 
     {
         options: ['option1', 'option2'],
         runsWhen: (args) => args.options.option3 !== undefined // dependant
     },
     {
         options: ['option3', 'option4'] // will run always default         
     },
     {
         options: ['option5', 'option6'],
         runsWhen: (args) => args.options.option5 || args.options.option6 // exclusive + optional options
     }
  ];
} 

[waldekmastykarz]

I like the idea a lot! Let's think what's the best way to implement this would be. Adding a new class-level getter would be the easy way to go about it, but I think it shows that we're hitting the limitations of the current design and would benefit from a more robust approach that allows us to better accommodate such requirements.

[waldekmastykarz]

Thinking more about the example you've provided: bucketId and bucketName are not just optional. Unless you specify title, you shouldn't be using either one at all, but when you do use title you must use either bucketId or bucketName. So it's more than an optional optionSet. It's an option set with a dependency on another property, and I wonder if there's an easy way to express such dependency declaratively.

[waldekmastykarz]

@milanholemans @Jwaegebaert any ideas/opinions? 😊

[Adam-it]

@martinlingstuyl good idea. I noticed this before when I even added a wrong review comment to use option set for this kind of requiredWhen options 😋. I agree we should search for a way to improve this 👍. Maybe we could specify some arrays with options to valid when some condition is passed 🤔. This could be any kind of condition made on passed options 🤔

[martinlingstuyl]

Thinking more about the example you've provided: bucketId and bucketName are not just optional. Unless you specify title, you shouldn't be using either one at all, but when you do use title you must use either bucketId or bucketName. So it's more than an optional optionSet. It's an option set with a dependency on another property, and I wonder if there's an easy way to express such dependency declaratively.

You're correct on this. It might not be the right example for the issue. Maybe for such a scenario we could offer DependentOptionSets, like @Adam-it suggests here:

@martinlingstuyl good idea. I noticed this before when I even added a wrong review comment to use option set for this kind of requiredWhen options 😋. I agree we should search for a way to improve this 👍. Maybe we could specify some arrays with options to valid when some condition is passed 🤔. This could be any kind of condition made on passed options 🤔

Maybe we need to extend OptionSet with a required function. You can write the function to return true in all cases, or just in the case where a certain option is defined. You could also return false, saying that the properties are not required, but if you specify one, you should not specify the other.

In this way we would cover all three scenario's:

1. Required OptionSet (like they are now)
2. Dependant OptionSet (like with planner task get)
3. Optional OptionSet (don't know where this applies right now, but it probably does somewhere, or will in the future)

[waldekmastykarz]

So specifying an option set would look like:

const optionSet: any[][] = [
  ['option1', 'option2'], // all required
  ['option3', 'option4', (args) => typeof args.options.option1 !== 'undefined'], // required when option1 set
]

In each option set we'd allow to set the last value to a function that determines if the option set should be used. If that function is not set, the option set is used. Is that what you mean @martinlingstuyl?

[martinlingstuyl]

That's exactly what I mean. And additionally, you could achieve optional OptionSets as follows:

const optionSet: any[][] = [
  ['option1', 'option2'], // all required
  ['option3', 'option4', (args) => typeof args.options.option1 !== 'undefined'], // required when option1 set
  ['option5', 'option6', () => false] // optional, but either one or the other
]

[martinlingstuyl]

In the case of planner task get you'd have bucketId/bucketName are dependant on title and planTitle/planId are dependant on bucketName I think. ownerGroupId/ownerGroupName are dependant on planTitle.

That would solve the entire riddle I think.

And it would lead to a lot less validation code to write yourself.

[martinlingstuyl]

We do have scenario's in which we have three options in an OptionSet I think, so we should take care to check the type of the last item in the array that it is of type function

[Jwaegebaert]

If I'm not mistaken @martinlingstuyl, then the options assignedToUserIds and assignedToUserNames available in the commands planner task set and planner task add would be part of the optional optionSet.

[Jwaegebaert]

If we are going to create three different types of optionSets here. Then it would be interesting to start building interfaces for them. This would create more overview and reduce error margin a bit. For example, the interfaces could look like this.

export interface OptionSets {
  required?: string[][];
  dependant?: DependantOptionSet[];
  optional?: string[][];
}

export interface DependantOptionSet {
  options: string[];  
  condition: () => boolean;
}

When we apply this to our optionSet constant, it could look like this.

const optionSet: OptionSets = {
  required: [['option1', 'option2'], ['option3', 'option4']], 
  dependant: [
    {
      options: ['option5', 'option6'],
      condition: () => typeof option2 !== 'undefined'
    },
    {
      options: ['option7', 'option8'],
      condition: () => typeof option6 !== 'undefined'
    }
  ],
  optional: [['option9', 'option10'], ['option11', 'option12']]
};

However, this structure will cause a lot of refactor work on the optionSets that @waldekmastykarz is working on.

[milanholemans]

If we are going to create three different types of optionSets here. Then it would be interesting to start building interfaces for them. This would create more overview and reduce error margin a bit. For example, the interfaces could look like this.

export interface OptionSets {
  required?: string[][];
  dependant?: DependantOptionSet[];
  optional?: string[][];
}

export interface DependantOptionSet {
  options: string[];  
  condition: () => boolean;
}

When we apply this to our optionSet constant, it could look like this.

const optionSet: OptionSets = {
  required: [['option1', 'option2'], ['option3', 'option4']], 
  dependant: [
    {
      options: ['option5', 'option6'],
      condition: () => typeof option2 !== 'undefined'
    },
    {
      options: ['option7', 'option8'],
      condition: () => typeof option6 !== 'undefined'
    }
  ],
  optional: [['option9', 'option10'], ['option11', 'option12']]
};

However, this structure will cause a lot of refactor work on the optionSets that @waldekmastykarz is working on.

This is also something I wanted to suggest to be honest. I like the idea of having 3 option sets, but it might become unclear if we put them all mixed together in a single array with option names and a condition. Especially for new contributors, if they don't know the logic behind it, it can get confusing quickly. I'm more of a fan of dividing things like this into objects with properties like @Jwaegebaert suggests This way we can also provide a clear description via IntelliSense what required optionSets are, what dependant optionSets are, ...

It will indeed cause a lot of rework on the PR @waldekmastykarz is working on, but that issue might be so big that we can split it between different people. I think that this kind of PRs should be merged asap because of the size of it. I'm sure when Waldek finishes this PR, the number of merge conflicts will be significant.

[martinlingstuyl]

I do think that to be a bit verbose. There is a way in between, for example like this:

public optionSets: OptionSet[]  {
  return [ 
     {
         options: ['option1', 'option2'],
         required: (args) => return args.options.option3 !== undefined // dependant
     },
     {
         options: ['option3', 'option4'] // required by default         
     },
     {
         options: ['option5', 'option6'],
         required: () => false // optional
     },
  ];
}

[milanholemans]

@martinlingstuyl instead of () => false, I tend to opt for a slightly easier model. Required can be either a boolean or a function.

public optionSets: OptionSet[]  {
  return [ 
     {
         options: ['option1', 'option2'],
         required: (args) => args.options.option3 !== undefined // dependant
     },
     {
         options: ['option3', 'option4'], // required by default
         required: true // Maybe use this as default value?
     },
     {
         options: ['option5', 'option6'],
         required: false // optional
     },
  ];
}

[martinlingstuyl]

Though I agree this is clearer, still it would take a lot of refactoring. Plus it's still much more verbose than using ['option1', 'option2', false] or ['option1', 'option2', (args) => args.options.option3 !== undefined].

[Adam-it]

TBH I think any kind of improvement we do in this area will be very positive for sure. I really like model separation between options and required field like @milanholemans suggested. For me it is very clear but of course it will introduce more refactoring that is true. BTW thanks @milanholemans and @Jwaegebaert in engaging in this discussion. Your contributions are tremendous 🤩🤩. You rock, As always 🙂

[waldekmastykarz]

Thank you for chiming in everyone: awesome to get your ideas!

Not defining specific types of option sets, like we discussed here:

export interface OptionSets {
  required?: string[][];
  dependant?: DependantOptionSet[];
  optional?: string[][];
}

and using a more generic approach with an array of option sets, each having an optional function to determine when it should run, gives us more flexibility for other cases and will likely be more future-proof.

Like I suggested initially and @martinlingstuyl agreed, having something like:

['option1', 'option2', (args) => args.options.option3 !== undefined]

is simple and not overly verbose, and wouldn't be a pain to repeat in case we have multiple option sets in a command. But the shortcoming this code has, and which you pointed out as well, is that it's "clever". It implicitly assumes that the last value is special and to enforce it on design time, we'd likely need a custom eslint rule, which begs the question if it's the right approach after all.

Looking at the different examples we've discussed, I'm thinking now about something like:

public optionSets: OptionSet[]  {
  return [ 
     {
         options: ['option1', 'option2'],
         shouldRun: (args) => args.options.option3 !== undefined // dependant
     },
     {
         options: ['option3', 'option4'] // will run always default         
     },
     {
         options: ['option5', 'option6'],
         shouldRun: (args) => args.options.option5 || args.options.option6 // exclusive + optional options
     }
  ];
}

It's a variation on other examples you've shared with a few differences:

• I renamed required and condition to shouldRun to more clearly depict its purpose. If an option set is depicted as required, what is it required for? shouldRun I think clearly states that the optionSet will be run when the condition is true
• in cases when we have options that are exclusive but you're not required to pass one, I define a custom validation condition (example with options 5 and 6). I think this is clearer than something like shouldRun: false, which could suggest that the option set is disabled

Again, thank you all for chiming in and your thoughts and looking forward to your feedback

[martinlingstuyl]

Hi Waldek, I agree with you that the less verbose version has it's downsides. I like your proposition as well. About the naming of the function: does runsWhen make even more sense?

runsWhen: (args) => args.options.option3 !== undefined runsWhen: (args) => args.options.option5 || args.options.option6

omitting it means run always

[waldekmastykarz]

Yup, runsWhen is a nice and clear way to express it too. I like it! 👏

[Jwaegebaert]

and using a more generic approach with an array of option sets, each having an optional function to determine when it should run, gives us more flexibility for other cases and will likely be more future-proof.

Well caught, that will be more convenient to use in the long run.

I'm also a fan of the naming runsWhen. Makes more sense, great suggestion @martinlingstuyl! If we have chosen a design pattern, it would be usefull to implement it asap. This in the manner that @milanholemans suggested for example.

It will indeed cause a lot of rework on the PR @waldekmastykarz is working on, but that issue might be so big that we can split it between different people. I think that this kind of PRs should be merged asap because of the size of it. I'm sure when Waldek finishes this PR, the number of merge conflicts will be significant.

If we can divide the work here among all of us then this can be implemented quickly without too many of those nasty merge conflicts. Divide and Conquer! 😄

[waldekmastykarz]

If we can divide the work here among all of us then this can be implemented quickly without too many of those nasty merge conflicts. Divide and Conquer! 😄

One way that we could consider is:

• refactor how we define and run option sets and refactor options sets that are already defined to the new structure, must be done before everything else by one person because of the dependency between calling the option set logic from the CLI runtime and commands where they're defined
• identify all other option sets that we excluded initially but which we could fit in the new approach
• split the work grouping commands per service/whatever makes sense to spread the load

Depending on what we decide in #3218, we might also want to wait with this refactoring until we've done #3218 or we'll remove tests that we'd need to add back later adding unnecessary work.

[martinlingstuyl]

I've updated the specs. Let's wait to see what the other issue is getting up to

[martinlingstuyl]

@waldekmastykarz, I think we'd best keep this one on hold till #3409 is done, right?

[waldekmastykarz]

Yes, you're right.

[milanholemans]

Blocking issue is completed, opening this issue back up again.

[waldekmastykarz]

Before we start implementation, we should split the feature in two parts:

1. Refactor the option sets interface, which needs to be done at once by one person because it's a breaking change
2. Define the list of commands where we could use the new functionality. We could define each command that needs to be changed as a separate issue so that we can invite other folks to help out as well. We can extend existing commands with new option sets functionality over time.