Advanced option specs (list types and required options)

[mpeck]

Motivation

Each command provides an option_spec that defines the options and args for that particular command. Currently we are limited to option types supported by getopt. Those types are: atom; binary; boolean; float; integer; string. getopt also has no concept of required options or arguments. To overcome these shortcomings we use a few functions defined in action_util; structured_options and convert_to_params. These functions provide a mechanism to support additional types, like list, and a way to note that an option or argument is required. This method works, but it requires the functions to be included in the entry point to every command. This leads to inconsistencies and extra boilerplate for every command.

Since this is a relatively minor but sweeping change I think it will be fine to include all of it in a single PR. There already exists functions that do the work, it's just in the wrong place. Most of the changes will be in Cogclt.Optparse but all of the command modules will contain minor updates.

Objective

We should only need to specify option/arg specifications in one place, option_spec. option_spec should additionally accept a list type. List is the only additional type that we currently require.

Research

There are two bits that need to be done. We need to modify option_spec to accept a required flag and move the logic that enforces required params to Cogctl.Optparse. We should also move the logic that provides additional option types to Cogctl.Optparse. Cogctl.Optparse invokes getopt to parse options. We can intercept the results before they are passed to the appropriate handler either modify them or display an appropriate error to the user.

getopt will fail if the option spec passed to it is not what it expects. Since we are adding some additional bits, we will need to process the option spec before passing it to getopt.

I'm thinking that our new improved option spec might look something like this (taken from relay groups):

    [{:relay_group, :undefined, :undefined, :string, 'Relay Group name', :required},
     {:relays, :undefined, 'relays', :list, 'Relay names', :required}]

There are two things to note. The last item in the option spec tuple is an optional :required atom and an additional option type :list is included. Prior to parsing options, we will process the spec, and create a valid option spec for getopt:

    [{:relay_group, :undefined, :undefined, :string, 'Relay Group name'},
     {:relays, :undefined, 'relays', :string, 'Relay names'}]

And a list of required fields and field type:

    [{:relay_group, :string, :required},
     {:relays, :list, :required}]

After parsing we can verify that we have all required options and create a proper elixir list for the list type. If a required option is not present we can send the appropriate error message to the user and shutdown.

Question: Do we have any thoughts on the syntax for lists? Currently we use a fairly naive approach and just split the string on ",". It works, but it does feel a bit awkward when entering; cogctl relay-groups create mygroup --members relay1,relay2,relay3,relay4. It feels more natural, and useful, to just use a space; cogctl relay-groups create mygroup --members relay1 relay2 relay3 relay4, but obviously there are other issues with that. Is there a standard for getting input like this? I've seen it done several ways.

Estimated level of effort: 1 day

Tasks

• Pre-process option spec before passing it to getopt
• Validate options after parsing, shutdown and return an error when a required option is missing
• If type is a list split on "," and put an elixir list in options
• Update commands to use the new option spec

[kevsmith]

I think we should leave lists as comma separated values for now to minimize work. We can revisit when we're ready to tackle cogctl 2.0.

👍

[mpeck]

Turns out getopt does support required options. It just doesn't mention them in the README. Any option that does not have a default value is considered required. You can check for required fields by running :getopt.check/2.

This does necessitate a tweak to getopt though. Currently getopt will error and only return the first missing required field it encounters. We would instead like to return all missing required fields. We need to patch getopt to return all missing fields and rework our opt parser to run :getopt.check/2 instead of a custom implementation.