Skip to content
This repository has been archived by the owner on Dec 10, 2022. It is now read-only.

config should be an option #53

Open
suhr opened this issue Mar 28, 2019 · 8 comments
Open

config should be an option #53

suhr opened this issue Mar 28, 2019 · 8 comments

Comments

@suhr
Copy link
Member

suhr commented Mar 28, 2019

This issue proposes to remove config subcommand in favour of --config option.

Motivation

An english dictionary gives the following definition of the word “subcommand”:

(computing) A command that makes up part of a larger command.
This command accepts additional subcommands as parameters.

In command line interfaces, subcommands are used to execute specific actions. Some examples of various subcommands:

cargo build --example=foo
git commit -m "Initial commit"
http get https://github.com/

At this moment, tox-node uses config subcommand for running a node with a configuration file, and command line options to run a node without a configuration file. The main drawback of this approach is that it violates the subcommand semantics: config is not an action. Beside that, this approach provides no way to override the parameters specified in the configuration file.

The distinction between the confiuration file option and other options was created to avoid the problem of mixing the configuration file with configuration options. This issue proposes a different solution: configuration merging.

Detailed design

  1. Remove the config subcommand
  2. Add the --config cli option instead
  3. Implement merging cli options with options in an configuration file

The left merge of configurations

A configuration A is a set of pairs (N, V), such as if (N, V_1) ∈ A and (N, V_2) ∈ A, than V_1 = V_2. The first element of the pair is called a name and the second is called a value. We will write configurations as {N_1 = V_1, N_2 = V_2}.

We say that name n is in A if there exists such v that (n, v) ∈ A.

A symmetric difference of configurations A and B is a configuration that contains pairs from both sets with names only in A or only in B. Symbolically, this is can be written as A ∆ B = {(n, v) | ((n, v) ∈ A ∧ (n, _) ∉ B) ∨ ((n, v) ∈ B ∧ (n, _) ∉ A)}.

A left merge of configurations A and B, written A < B, is defined as this:

  1. A ∆ B ⊂ A < B
  2. If (n, a) ∈ A and (n, b) ∈ B than (n, b) ∈ A < B

Proposition: a left merge is a monoid with empty configuration as the identity.

Indeed:

  1. {} < A = A < {} = A, since A ∆ {} = A
  2. ({x = a} < {x = b}) < {x = c} = {x = a} < ({x = b} < {x = c}) = {x = c}

Merging the configuration file with cli options

The result of merging the configuration file with cli options is the left merge of configurations. In other words, cli options have a priority over the configuration file, so when a parameter is defined by both the configuration file and a cli option, the cli option overrides parameter specified in the configuration file.

This allows to use the same configuration file for different configurations of the tox node. For example, a user may change the used key file with --keys-file option, without changing the configuration file.

Alternatives

  • Deprecate the config subcommand instead of removing
  • Do not merge the configuration, instead return an error when the --config option is used with other options.

How do we teach this?

The README.md already describes config to be an option instead of a subcommand.

Drawbacks

  • The change breaks scripts that use config as a subcommand
  • Slight increase of cli code complexy because of configuration merging
@kurnevsky
Copy link
Member

kurnevsky commented Mar 28, 2019

The main drawback of this approach is that it violates the subcommand semantics: config is not an action.

It can be considered as action "read config and run node", when default action is "run node". Your example with cargo demonstrates exact same thing - cargo test builds project first with cargo build --tests and only then runs tests. git also has such commands, e.g. git pull = git fetch + git merge. So it's not very clear when one command ends and another begins.

Beside that, this approach provides no way to override the parameters specified in the configuration file.

Do we really need it? Also there is another feature that often present in unix utils: arguments that are supposed to be unique can be specified multiple times. This way later arguments override earlier ones. This allows to alias commands with arguments and then override these arguments running aliases. Also to be able to override flags there are reversed flags, like --arg and --no-arg. So how far should we go? I have an impression that all this is contrary to the clap's approach.

Proposition: a left merge is a monoid with empty configuration as the identity.

This might be confusing for lists. For example, we have bootstrap nodes list and one might expect that specifying node via CLI will add it to the list but not override the whole list of nodes specified in the config.

Drawbacks

  • The change breaks scripts that use config as a subcommand
  • Slight increase of cli code complexy because of configuration merging

There is one more major drawback - this way we won't be able to use clap's validation and default arguments declaration. We'll have to assume that all CLI parameters are optional because they can be specified in config. So we'll have to check each required parameter manually and if it's not present in both config and CLI we'll have to panic. That was the reason why we used subcommand in the first place.

@suhr
Copy link
Member Author

suhr commented Mar 28, 2019

So it's not very clear when one command ends and another begins.

Still there's a very clear generic action (test the project, pull from a remote), unlike config.

There is one more major drawback - this way we won't be able to use clap's validation.

I see. Well, then it's probably easier to just return an error when --config is used with other options.

@kurnevsky
Copy link
Member

kurnevsky commented Mar 28, 2019

Still there's a very clear generic action (test the project, pull from a remote), unlike config.

But what if it was called run-with-config? Also can we name it with dashes, just like --config argument?

I see. Well, then it's probably easier to just return an error when --config is used with other options.

This also messes with parsing a bit. --config will have to conflict with all arguments that don't have default value. Also instead of require we will have to use require_unless("--config") (though we don't have much requires right now).

@kpp
Copy link
Member

kpp commented Mar 28, 2019

This allows to use the same configuration file for different configurations of the tox node. For example, a user may change the used key file with --keys-file option, without changing the configuration file.

This is clear for --keys-file, how about --bootstrap-node?
How about default values? --threads = 1 if nothing is set.

@suhr
Copy link
Member Author

suhr commented Mar 29, 2019

Also can we name it with dashes, just like --config argument?

Sounds like a hack.

How about default values? --threads = 1 if nothing is set.

Defaults < configuration file < cli arguments.

@kpp
Copy link
Member

kpp commented Mar 29, 2019

Defaults < configuration file < cli arguments.

Why? When there could be an easy way to use only Defaults < configuration file. Because simple logic > complicated logic.

@suhr
Copy link
Member Author

suhr commented Mar 29, 2019

Yes, I agree that this kind of logic is probably too complicated. So the most reasonable solution is indeed to forbid using --config with other arguments.

@kpp
Copy link
Member

kpp commented Mar 29, 2019

This is possible with https://docs.rs/clap/2.32.0/clap/struct.Arg.html#method.conflicts_with_all , but we will have to list every arg in conflicts_with_all(&[arg1, arg2, arg3]) and add require_unless("--config") for required args. Or ask clap-rs to add a method named unique for such args =)

It will bloat the code, so we have to cover it with tests.

But what if it was called run-with-config?

How about that? or with-config?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants