(WIP) Support configuration file #312

nrskt · 2022-03-17T13:24:15Z

This pull request is a draft of the supporting configuration file.
I'm worried about implementations as follows.

two kinds of types of configuration files are supported.
- $XDG_CONFIG_HOME/tokio-console/console.toml
- current configuration (./console.toml)

I want some advice about these implementations.
(Should I rewrite clap builder api style?)

Relates to #310

nrskt · 2022-03-17T13:37:05Z

tokio-console/src/config.rs

@@ -28,7 +32,7 @@ pub struct Config {
    #[clap(long = "log", env = "RUST_LOG", default_value = "off")]
    pub(crate) env_filter: tracing_subscriber::EnvFilter,

-    #[clap(flatten)]
+    #[clap(skip)]


The reason I skipped it:

I don't know how to use default values from config files.

If the parameter is enabled, I should implement that:

If command-line options are given, then need to use them instead of the config file's value.

nrskt · 2022-03-17T13:48:59Z

tokio-console/src/config.rs

+    pub fn from_config() -> color_eyre::Result<Self> {
+        let xdg_config_path = env::var_os("XDG_CONFIG_HOME").map(|mut base| {
+            base.push("/tokio-console/console.toml");
+            base
+        });
+        let xdg_view_opt = xdg_config_path.and_then(ViewOptions::from_config);
+        let current_view_opt = ViewOptions::from_config("console.toml");
+
+        let config = Config::parse();
+
+        match xdg_view_opt.or(current_view_opt) {
+            None => Ok(config),
+            Some(view_opt) => Ok(Self {
+                view_options: view_opt,
+                ..config
+            }),
+        }
+    }


I'm worried:

I don't know woking it on macOS or Windows.

Some rules are broken.

ex) it can use --palette and --colorterm.

I don't know woking it on macOS or Windows.

I think we might want to look into using the dirs crate for reading configs from the correct location on multiple platforms. That library has a config_dir function that returns the correct operating-system-specific config file location.

On macOS, we might also want to additionally check for ~/.console.toml (and maybe also ~/.config/console.toml), which are both Linux style locations --- although Apple suggests the use of ~/Library/Application Support for config files, a lot of people still like to put config files for command-like utilities in ~ or ~.config/...

config_dir is exactly what i have wanted. I'll use it.

hawkw

This looks like a good start. I left some suggestions inline.

One higher-level thought is that I'm not sure if it really makes sense to have one set of structs that implements both serde::Deserialize and clap::Parser. We might want to structure the command-line arguments and configuration files differently, so we may want to define separate types for reading configs from the command line and from a file, and then combine them into one internal config type that's used to actually configure the console.

On the other hand, it's possible that we can use #[clap(...)] and #[serde(...)] attributes to implement any differences between the structure of the command-line arguments and the config file. If this is practical, it might be a bit nicer to not have to convert between multiple structs! But, if the best solution requires us to have separate structs for the CLI and config file, I think that's fine.

hawkw · 2022-03-17T16:39:44Z

console.toml

+no_colors = false
+lang = "en_us.UTF-8"
+ascii_only = true
+palette = "8"
+
+
+[toggles]
+color_durations = true
+color_terminated = true


Was this checked in for testing purposes? Eventually, I think it could be good to have an example config file with the default options (and comments explaining them) checked into the repo, so that users can see how to configure the console. But, I don't know if it should be "live" --- it might be better to name the example something like "console.example.toml" or similar, so that running the console from the repo directory doesn't actually use those settings...

Sorry I added it unintentionally.
Eventually, I provide an example config file or add the explanation to README.

hawkw · 2022-03-17T16:47:28Z

console.toml

+no_colors = false
+lang = "en_us.UTF-8"
+ascii_only = true
+palette = "8"


not sure if it makes sense for the config file to have separate keys for no_colors and palette, if we can also write palette = "off" to disable colors. i'd prefer to not have two separate ways to write the same thing in the config file.

hawkw · 2022-03-17T16:51:01Z

console.toml

+no_colors = false
+lang = "en_us.UTF-8"
+ascii_only = true
+palette = "8"
+
+
+[toggles]
+color_durations = true
+color_terminated = true


I'm not sure if it makes sense to match the layout of the config structs exactly in the config file. instead, i think it might be good to group the different configurations into TOML tables, maybe something like this:

[charset] lang = "en_us.UTF-8" ascii_only = false [colors] enabled = true palette = "8" [colors.enable] durations = true terminated = true

or something like that?

That sounds good, I'll try it.

hawkw · 2022-03-17T16:56:57Z

tokio-console/src/config.rs

+        match xdg_view_opt.or(current_view_opt) {
+            None => Ok(config),
+            Some(view_opt) => Ok(Self {
+                view_options: view_opt,
+                ..config
+            }),
+        }


eventually, I think what we'll want is a function on the Config type to merge two Configs, with one overriding the other. I think the behavior we actually want here is something like this:

read the config file in the platform-specific config location (e.g. XDG_CONFIG_HOME) as the "base" config, or use the defaults if there is no config file there

if there is a config file in the current directory, any configs set in that config file override the settings in the "base" config

any command-line arguments that are set override the configs from both the current directory and the base directory

does that make sense?

Yes, that makes sense. Ok, I'll try it again.

hawkw · 2022-03-17T17:00:13Z

tokio-console/src/config.rs

+    pub fn from_config() -> color_eyre::Result<Self> {
+        let xdg_config_path = env::var_os("XDG_CONFIG_HOME").map(|mut base| {
+            base.push("/tokio-console/console.toml");
+            base
+        });
+        let xdg_view_opt = xdg_config_path.and_then(ViewOptions::from_config);
+        let current_view_opt = ViewOptions::from_config("console.toml");
+
+        let config = Config::parse();
+
+        match xdg_view_opt.or(current_view_opt) {
+            None => Ok(config),
+            Some(view_opt) => Ok(Self {
+                view_options: view_opt,
+                ..config
+            }),
+        }
+    }


I don't know woking it on macOS or Windows.

I think we might want to look into using the dirs crate for reading configs from the correct location on multiple platforms. That library has a config_dir function that returns the correct operating-system-specific config file location.

On macOS, we might also want to additionally check for ~/.console.toml (and maybe also ~/.config/console.toml), which are both Linux style locations --- although Apple suggests the use of ~/Library/Application Support for config files, a lot of people still like to put config files for command-like utilities in ~ or ~.config/...

nrskt · 2022-03-18T11:23:54Z

@hawkw Thank you for reviewing.

We might want to structure the command-line arguments and configuration files differently, so we may want to define separate types for reading configs from the command line and from a file, and then combine them into one internal config type that's used to actually configure the console.

Your input has inspired me to change my implementation approach.

I'll try as follows:

Define the type of config from a file.
Define merge config function.

On the other hand, I have a problem.

read the config file in the platform-specific config location (e.g. XDG_CONFIG_HOME) as the "base" config, or use the defaults if there is no config file there

if there is a config file in the current directory, any configs set in that config file override the settings in the "base" config

any command-line arguments that are set override the configs from both the current directory and the base directory

I can implement the first and second points, but I am having difficulty with the third suggestion.
because I don't know how to identify the command-line argument set when using clap::Parser::parse() was used. I think if the value does not set, then use the default value that defines by #[clap(...)]. So, I don't know what value should be overridden.

Do you have any ideas in this regard?

nrskt · 2022-03-21T11:21:53Z

I'm trying to rewrite the config parser using clap Builder-API style. It will take some time because I'm doing it while checking to document it.

Maybe I'll submit it as another Pull Request. (Should I close this PR?)

hawkw · 2022-03-22T17:27:31Z

I can implement the first and second points, but I am having difficulty with the third suggestion. because I don't know how to identify the command-line argument set when using clap::Parser::parse() was used. I think if the value does not set, then use the default value that defines by #[clap(...)]. So, I don't know what value should be overridden.

Do you have any ideas in this regard?

I think the simplest solution would be to just change the structs that derive clap::Parser so that all their values are Options, and remove any default values from the struct. Then, we would merge configurations by using any values that are Some in the command line arguments, and falling back to the config file for anything that is None in the command-line arguments.

I don't think this will require the use of the clap builder API.

nrskt · 2022-03-24T01:52:33Z

I think the simplest solution would be to just change the structs that derive clap::Parser so that all their values are Options, and remove any default values from the struct.

That sounds great! I'll try your suggestion.

nrskt · 2022-04-05T15:37:45Z

@hawkw Hi, I create a new pull request about this PR. please review it.

#320

Support config files as follows. - $XDG_CONFIG_HOME/tokio-console/console.toml - current configuration (`./console.toml`) ## Test Execute and check debug log. `env -u LANG -u COLORTERM RUST_LOG=debug cargo run` ### case1) no config files ``` DEBUG tokio_console: args.target_addr=http://127.0.0.1:6669/ args.view_options= ViewOptions { no_colors: None, lang: None, ascii_only: None, truecolor: None, palette: None, toggles: ColorToggles { color_durations: None, color_terminated: None } } ``` ### case2) current config file ```toml [charset] lang = "en_us.UTF-8" ascii_only = true [colors] enabled = true palette = "all" truecolor = true [colors.enable] durations = true terminated = true ``` ``` DEBUG tokio_console: args.target_addr=http://127.0.0.1:6669/ args.view_options= ViewOptions { no_colors: Some(false), lang: Some("en_us.UTF-8"), ascii_only: Some(true), truecolor: Some(true), palette: Some(All), toggles: ColorToggles { color_durations: Some(false), color_terminated: Some(false) } } ``` ### case3) XDG config file ```toml [charset] lang = "en_us.UTF-8" ascii_only = true [colors] enabled = true palette = "all" truecolor = true [colors.enable] durations = true terminated = true ``` ``` DEBUG tokio_console: args.target_addr=http://127.0.0.1:6669/ args.view_options= ViewOptions { no_colors: Some(false), lang: Some("en_us.UTF-8"), ascii_only: Some(true), truecolor: Some(true), palette: Some(All), toggles: ColorToggles { color_durations: Some(false), color_terminated: Some(false) } } ``` ### case4) XDG config file and current config file XDG_CONFIG/console.toml ```toml [charset] lang = "en_us.UTF-8" ascii_only = true [colors] enabled = true palette = "all" truecolor = true [colors.enable] durations = true terminated = true ``` ./console.toml ``` [charset] lang = "en_us.UTF-8" ascii_only = false ``` Override the current config file. ``` DEBUG tokio_console: args.target_addr=http://127.0.0.1:6669/ args.view_options= ViewOptions { no_colors: Some(false), lang: Some("en_us.UTF-8"), ascii_only: Some(false), <-- override truecolor: Some(true), palette: Some(All), toggles: ColorToggles { color_durations: Some(false), color_terminated: Some(false) } } ``` ### case5) XDG config file, current config file, and command-line option XDG_CONFIG/console.toml ```toml [charset] lang = "en_us.UTF-8" ascii_only = true [colors] enabled = true palette = "all" truecolor = true [colors.enable] durations = true terminated = true ``` ./console.toml ``` [charset] lang = "en_us.UTF-8" ascii_only = false ``` `env -u LANG -u COLORTERM RUST_LOG=debug cargo run -- --palette 8` Override the current config file and command-line option. ``` DEBUG tokio_console: args.target_addr=http://127.0.0.1:6669/ args.view_options= ViewOptions { no_colors: Some(false), lang: Some("en_us.UTF-8"), ascii_only: Some(false), <-- override truecolor: Some(true), palette: Some(Ansi8), <-- override toggles: ColorToggles { color_durations: Some(false), color_terminated: Some(false) } } ``` Relates to - #310 - #312 - #312 (comment) Closes #310

WIP

2ea4de6

nrskt commented Mar 17, 2022

View reviewed changes

hawkw reviewed Mar 17, 2022

View reviewed changes

nrskt mentioned this pull request Apr 5, 2022

feat(console): Support config file #320

Merged

nrskt closed this Apr 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(WIP) Support configuration file #312

(WIP) Support configuration file #312

nrskt commented Mar 17, 2022 •

edited

Loading

nrskt Mar 17, 2022

nrskt Mar 17, 2022

hawkw Mar 17, 2022

nrskt Mar 18, 2022

hawkw left a comment

hawkw Mar 17, 2022

nrskt Mar 18, 2022

hawkw Mar 17, 2022

hawkw Mar 17, 2022

nrskt Mar 18, 2022

hawkw Mar 17, 2022

nrskt Mar 18, 2022

hawkw Mar 17, 2022

nrskt commented Mar 18, 2022

nrskt commented Mar 21, 2022

hawkw commented Mar 22, 2022

nrskt commented Mar 24, 2022

nrskt commented Apr 5, 2022

(WIP) Support configuration file #312

(WIP) Support configuration file #312

Conversation

nrskt commented Mar 17, 2022 • edited Loading

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

hawkw 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

Choose a reason for hiding this comment

nrskt commented Mar 18, 2022

nrskt commented Mar 21, 2022

hawkw commented Mar 22, 2022

nrskt commented Mar 24, 2022

nrskt commented Apr 5, 2022

nrskt commented Mar 17, 2022 •

edited

Loading