Change how default config values are handed in the code

[chipbuster]

The most common way to handle default values for config options seems to be to attempt to look up the key in the toml, then assign a default value if that fails, something like config.get_as_type(key).unwrap_or(default_value). There are several potential problems I can see with this approach:

• It allows the user to typo keys in their config and not be warned about it (e.g. use_colour)
• It allows the programmer to typo keys in their code and not be warned about it (granted this should usually be caught on testing/code review)
• If the documentation and code have accidentally diverged, you have to go to the source code of the module to look up the correct default arguments.

A possible alternative would be to create a config manager class that holds enums of all the config options, along with their default values. You would then be able to query it by asking it for the value of the enum, e.g. we would replace

    let signed_config_min = module.config_value_i64("min_time").unwrap_or(2);

with

   let signed_config_min = module.get_config_value(config::min_time);

We could even start to encode things like implied arguments (if one value is specified in the config, then multiple other values are implicitly set to certain values), or incompatible arguments across modules (if doing A in one module, cannot do B in another) into this config manager, and give starship the ability to check the toml for correctness.

Advantages:

• Code that accesses invalid configuration options will not compile
• Since code now knows about all config options, it can check user's toml file for inconsistencies or invalid config values
• Simplifies reading in of options in module code

Disadvantages:

• Large initial pain to switch over
• Cannot immediately see default value from within module code
• Potential unseen complexities in implementation

Would this seem like a net positive for us, or does it seem closer to neutral for what we have at the moment?

[matchai]

I'm absolutely a fan of this, but I think we need to do some planning to make sure we're not locking in on something that would limit extensibility.

The main reason I didn't decide to go for this from the get-go is because I had other ideas around where I thought configuration might be headed. The end-goal I had in mind was to have all segments also be configurable in the same way modules are.

Providing a string value as a segment's configuration would set that as its "value" while providing a table would allow for very granular configuration (style, value, segment-specific configuration).

Here's one such example of what I had in mind, taken from spaceship-prompt/spaceship-prompt#397:

[git_status]
prefix = false
suffix = false

	[git_status.added]
	value = "•"
	style = "yellow"

	[git_status.untracked]
	value = "•"
	style = "blue"

	[git_status.deleted]
	value = "•"
	style = "green"

	[git_status.modified]
	value = "•"
	style = "red"

Working with enums would certainly still be possible, but it would make it quite a bit more complex. What are your thoughts on the above configuration approach?

[chipbuster]

Mmmm.

I definitely like the idea of configuring segments as well. I think doing it with enums would be possible, but it would depend on how good the support for nested enums in Rust looks like (a quick google search doesn't give me a solid idea--seems like not many people use these things).

Whether we use strings as keys or enums as keys doesn't affect the part where the config framework can check config files and provide default values for us...so I guess the big question is whether the ergonomics of enums are good enough to allow us to do this without too much trouble (and of course questions about design flexibility of switching to this system)

[heyrict]

We can create a folder named configs (or something else to avoid naming conflicts with config.rs) which has the same structure with modules. Each file looks like this:

// src/configs/character.rs
pub struct /* or enum */ CharacterConfig { ... };
impl ModuleConfig<CharacterConfig> for CharacterConfig {...};

pub const DEFAULT_CHARACTER_CONFIG: CharacterConfig = CharacterConfig {...};

where

// Somewhere
pub trait ModuleConfig {
  fn load_config(&self, &toml::Value::Table) -> Self;
}

or we can use some macros like #[derive(ModuleConfig)] for simple configs (I don't have any experience using macros, but I think it is possible). Implementing derive macros for enums may be somewhat complex, imo.

This should avoid accessing invalid options, simplify reading options and even make it possible to generate a default starship.toml file.

[bijancn]

enums are certainly better than Strings for problems that have a well defined number of cases like configuration to improve testability, reduce bugs and allow for better error handling. For nesting, we could simply nest the modules like this

mod config {
    pub enum GitStatus {Prefix, Suffix}
    pub mod git_status {
        pub enum Added {Value, Style}
        pub enum Untracked {Value, Style}
        pub enum Deleted {Value, Style}
        pub enum Modified {Value, Style}
    }
}
fn test() {
    print(config::GitStatus::Prefix);
    print(config::git_status::Added::Value);
}

However, already from this small example we can see that boilerplate is introduced and while it reduces some problems, what you would really want is to directly convert to a data class / struct like this

struct GitStatus {
    value: String,
    style: String,
}
struct GitConfig {
  prefix: Boolean,
  suffix: Boolean,
  added: GitStatus,
  untracked: GitStatus,
  deleted: GitStatus,
  modified: GitStatus
}

This layer should follow directly after reading the user configuration, which may only set a subset of the GitConfig and fills in the default values. To help with typos in user configuration, usage of unknown keys should show an error/warning. For avoiding boilerplate mistakes in the first layer, the #[derive(Deserialize)] macro could be really helpful https://docs.rs/toml/0.5.3/toml/

To make sure that the pattern is followed by all modules, we could setup a trait and every module is an implementation of that trait.