feat(console): Support config file #320

nrskt · 2022-04-05T15:36:19Z

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

[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

[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

[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

[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

- fix command line parser - fix config file parser

hawkw

thanks for looking on this, this looks like a good start! i had some suggestions.

hawkw · 2022-04-05T15:46:39Z

tokio-console/src/config.rs

@@ -163,11 +206,43 @@ impl Config {
    }
 }

+fn merge_view_options(base: &mut ViewOptions, command_line: ViewOptions) {


nit. take it or leave it: IMO, this seems like it would make more sense as a method on ViewOptions:

impl ViewOptions { fn merge_with(&mut self, command_line: ViewOptions) { // ... } }

this way, it's clearer which is the base and which is the override.

hawkw · 2022-04-05T15:49:07Z

tokio-console/src/config.rs

 // === impl ViewOptions ===

 impl ViewOptions {
    pub fn is_utf8(&self) -> bool {
-        self.lang.ends_with("UTF-8") && !self.ascii_only
+        let lang = self.lang.clone().unwrap_or_default();


i don't think we need to clone self.lang here; we can borrow it instead:

Suggested change

let lang = self.lang.clone().unwrap_or_default();

let lang = self.lang.as_ref().unwrap_or_default();

hawkw · 2022-04-05T15:51:05Z

tokio-console/src/config.rs

+#[derive(Debug, Clone, Deserialize)]
+pub struct ColorsEnable {
+    durations: bool,
+    terminated: bool,
 }


this struct seems very similar to ColorToggles, is it really necessary to have separate ColorToggles and ColorsEnable structs? can we just derive Deserialize for ColorToggles, and continue inverting the command-line arguments as part of the parse function when parsing CLI args?

this struct seems very similar to ColorToggles, is it really necessary to have separate ColorToggles and ColorsEnable structs?

No, it does not necessary. I think it can be fixed as you suggested.
I made a mistake that durations and terminated are required in the config file.

can we just derive Deserialize for ColorToggles, and continue inverting the command-line arguments as part of the parse function when parsing CLI args?

I think to invert the bool value with the getter method.

// === impl ColorToggles === impl ColorToggles { pub fn color_durations(&self) -> bool { self.color_durations.map(std::ops::Not::not).unwrap_or(true) } pub fn color_terminated(&self) -> bool { self.color_durations.map(std::ops::Not::not).unwrap_or(true) } }

Why I remove parse(from_flag = std::ops::Not::not) attribute.

First, I tried this approach.

I changed the fields (color_durations, color_terminated) to optional.
But, this approach does not pass the compile. because from_flag required fn(bool) -> T.

#[derive(Clap, Debug, Copy, Clone)] pub struct ColorToggles { /// Disable color-coding for duration units. #[clap(long = "no-duration-colors", parse(from_flag = std::ops::Not::not), group = "colors")] pub(crate) color_durations: Option<bool>, /// Disable color-coding for terminated tasks. #[clap(long = "no-terminated-colors", parse(from_flag = std::ops::Not::not), group = "colors")] pub(crate) color_terminated: Option<bool>, }

So I defined the getter method.

Okay, that seems fine to me, then!

hawkw · 2022-04-05T15:55:31Z

tokio-console/src/config.rs

+#[derive(Debug, Clone, Deserialize)]
+pub struct ConfigFile {
+    charset: Option<CharsetConfig>,
+    colors: Option<ColorsConfig>,
+}
+
+#[derive(Debug, Clone, Deserialize)]
+pub struct CharsetConfig {
+    lang: String,
+    ascii_only: bool,
+}
+
+#[derive(Debug, Clone, Deserialize)]
+pub struct ColorsConfig {
+    enabled: bool,
+    truecolor: bool,
+    palette: Palette,
+    enable: ColorsEnable,
+}


It looks like the the charset and colors fields are Options, but the fields on those types are all not optional. So, if I understand correctly, it looks like we can successfully parse a config file that does not include a charset section or a colors section, but if those sections are present, all their fields must be included in the config file.

For example, I believe a config file that contains only

[colors] enable = true;

will result in a parse error, because all the fields on ColorsConfig are required.

We should probably change this so that we can accept config files that don't set all the config file keys. So, we should probably make all the fields on these types Options.

I understood your suggestion. I thought the config file does not accept partial settings.
I'll fix it.

hawkw · 2022-04-05T15:57:47Z

tokio-console/src/config.rs

+    if command_line.no_colors.is_some() {
+        base.no_colors = command_line.no_colors;
+    }


nit, take it or leave it: i think we could potentially simplify these using Option::or:

Suggested change

if command_line.no_colors.is_some() {

base.no_colors = command_line.no_colors;

}

base.no_colors = command_line.no_colors.or(base.no_colors.take());

hawkw · 2022-04-05T16:01:53Z

tokio-console/src/config.rs

+// === impl ColorToggles ===
+
+impl ConfigFile {
+    fn from_config() -> Option<Self> {


can we change this to return a Result<Option<Self>, ...>? I think that if parsing the either config file fails, we should print the error to the user. of course, if there just weren't any config files present, we should still return an Option.

Ok, Certainly it is easy for users to understand.
I'll fix it.

hawkw · 2022-04-05T16:07:19Z

tokio-console/src/config.rs

+        if let Some(path) = base.as_mut() {
+            path.push("tokio-console/console.toml");
+        }
+        let base = base.and_then(|path| fs::read_to_string(path).ok());
+        let base_file: Option<ConfigFile> = base.and_then(|raw| toml::from_str(&raw).ok());


i think we could simplify this a bit by moving more of this code into the if let. here's my suggestion (assuming that we change this method to return a Result, as I suggested)

Suggested change

if let Some(path) = base.as_mut() {

path.push("tokio-console/console.toml");

}

let base = base.and_then(|path| fs::read_to_string(path).ok());

let base_file: Option<ConfigFile> = base.and_then(|raw| toml::from_str(&raw).ok());

let base_file = if let Some(path) = base.as_mut() {

path.push("tokio-console/console.toml");

fs::read_to_string(path).ok()

.map(|raw| toml::from_str::<ConfigFile>(&raw))

.transpose()?

} else {

None

}

hawkw · 2022-04-05T16:07:59Z

tokio-console/src/config.rs

+    }
+}
+
+fn merge_config_file(before: Option<ConfigFile>, after: Option<ConfigFile>) -> Option<ConfigFile> {


nit, take it or leave it: again, this seems like it could be better off as a method on ConfigFile?

nrskt · 2022-04-09T10:34:00Z

@hawkw

I fixed the implementations and try to test again.
I have questions about case7 and case8. please check it.

test

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

[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

[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

[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

[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) 
    } 
}

Added

case6) XDG config file, current config file

XDG_CONFIG/console.toml

[colors]
palette = "8"

./console.toml

[charset]
lang = "C"

DEBUG tokio_console: args.target_addr=http://127.0.0.1:6669/ args.view_options=
ViewOptions { 
    no_colors: None, 
    lang: Some("C"), 
    ascii_only: None, 
    truecolor: None, 
    palette: Some(Ansi8), 
    toggles: ColorToggles { 
        color_durations: None, 
        color_terminated: None 
    }
}

case7) Parse error

[Question] I want you to tell me how to output a better error message.

XDG_CONFIG/console.toml

[colors]
palette = "8"

./console.toml

[charset]
ascii_only = "string value"

❯ env -u LANG -u COLORTERM RUST_LOG=debug cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.06s
     Running `target/debug/tokio-console`
failed to parse config file: Error {
    msg: "failed to parse Some(\"./console.toml\")",
    source: Error {
        inner: ErrorInner {
            kind: Custom,
            line: Some(
                1,
            ),
            col: 13,
            at: Some(
                23,
            ),
            message: "invalid type: string \"string value\", expected a boolean",
            key: [
                "charset",
                "ascii_only",
            ],
        },
    },
}

case8) Ignore unknown config field

[Question] Did you expect this behavior?
I don't know how to check strictly.

XDG_CONFIG/console.toml

[colors]
palette = "8"

./console.toml

[charset]
ascii_only = false
wrong_key = false

DEBUG tokio_console: args.target_addr=http://127.0.0.1:6669/ args.view_options=
ViewOptions { 
    no_colors: None, 
    lang: None, 
    ascii_only: Some(false), 
    truecolor: None, 
    palette: Some(Ansi8), 
    toggles: ColorToggles { 
        color_durations: None, 
        color_terminated: None
    }
}

hawkw

This looks really close! I commented on a way to improve the error handling for parse errors, which you asked about in #320 (comment). I also had some small code style suggestions.

hawkw · 2022-04-11T17:07:11Z

tokio-console/src/config.rs

+            .and_then(|path| fs::read_to_string(path).ok())
+            .map(|raw| toml::from_str::<ConfigFile>(&raw))
+            .transpose()
+            .wrap_err_with(|| format!("failed to parse {:?}", path.into_path()))?;


A small note: I think the path will be formatted nicer if we use Path::display here instead of its Debug implementation:

Suggested change

.wrap_err_with(|| format!("failed to parse {:?}", path.into_path()))?;

.wrap_err_with(|| format!("failed to parse {}", path.into_path().display()))?;

hawkw · 2022-04-11T17:20:22Z

tokio-console/src/main.rs

+    let mut args = match config::Config::from_config() {
+        Err(e) => {
+            eprintln!("failed to parse config file: {:#?}", e);
+            std::process::exit(1);
+        }
+        Ok(args) => args,
+    };


I think that any config parsing error will be displayed more nicely if we format the error with its standard Debug ({:?}) formatter rather than its alternate-mode Debug formatter ({:#?}), according to the eyre documentation. So, if we change this to

Suggested change

let mut args = match config::Config::from_config() {

Err(e) => {

eprintln!("failed to parse config file: {:#?}", e);

std::process::exit(1);

}

Ok(args) => args,

};

let mut args = match config::Config::from_config() {

Err(e) => {

eprintln!("failed to parse config file: {:?}", e);

std::process::exit(1);

}

Ok(args) => args,

};

we'll get a much nicer message.

The documentation also notes that

Note that this is the representation you get by default if you return an error from fn main instead of printing it explicitly yourself.

So, we could also just write this and get the same output, which I think is much nicer:

Suggested change

let mut args = match config::Config::from_config() {

Err(e) => {

eprintln!("failed to parse config file: {:#?}", e);

std::process::exit(1);

}

Ok(args) => args,

};

let mut args = config::Config::from_config()?;

When I make either of those changes and run the console with the invalid configuration file from your example, I get output like this, which I think is more helpful than the :#? output:

:; cargo run Compiling tokio-console v0.1.3 (/home/eliza/Code/console/tokio-console) Finished dev [unoptimized + debuginfo] target(s) in 5.66s Running `target/debug/tokio-console` Error: failed to parse Some("./console.toml") Caused by: invalid type: string "string value", expected a boolean for key `charset.ascii_only` at line 2 column 14 Location: tokio-console/src/config.rs:337:14

(unfortunately, the output isn't colored, because we don't install our custom eyre handler until after we've parsed the config file...but there's not really a way around that, so this is good enough!)

Thanks, I have tried it and getting the expected result. c87ddfc

❯ RUST_LOG=debug cargo run Finished dev [unoptimized + debuginfo] target(s) in 0.05s Running `target/debug/tokio-console` Error: failed to parse ./console.toml Caused by: invalid type: string "string value", expected a boolean for key `charset.ascii_only` at line 2 column 14 Location: tokio-console/src/config.rs:337:14

hawkw · 2022-04-11T18:16:40Z

tokio-console/src/config.rs

+
+#[derive(Debug, Clone, Copy)]
+enum ConfigPath {
+    Xdg,


This is a minor nitpick, but I don't think Xdg is the best thing to call this variant. The dirs trait uses XDG_CONFIG_HOME on Linux, but it uses a different directory on Windows or macOS. Calling it Xdg kind of suggests to the reader that it is specifically using XDG_CONFIG_HOME...maybe we should call it something like User or Home or something

surely, It's right. I choose Home.
2da4fdf

hawkw · 2022-04-11T18:17:21Z

tokio-console/src/config.rs

+        self.color_durations.map(std::ops::Not::not).unwrap_or(true)
+    }
+
+    pub fn color_terminated(&self) -> bool {
+        self.color_durations.map(std::ops::Not::not).unwrap_or(true)


minor style nit: Not is imported, so we could just write

Suggested change

self.color_durations.map(std::ops::Not::not).unwrap_or(true)

}

pub fn color_terminated(&self) -> bool {

self.color_durations.map(std::ops::Not::not).unwrap_or(true)

self.color_durations.map(Not::not).unwrap_or(true)

}

pub fn color_terminated(&self) -> bool {

self.color_durations.map(Not::not).unwrap_or(true)

it might also be worth having a comment or something here explaining why these are inverted?

hawkw · 2022-04-11T18:25:24Z

tokio-console/src/config.rs

+    fn merge_with(&mut self, command_line: ViewOptions) {
+        self.no_colors = command_line.no_colors.or_else(|| self.no_colors.take());
+        self.lang = command_line.lang.or_else(|| self.lang.take());
+        self.ascii_only = command_line.ascii_only.or_else(|| self.ascii_only.take());
+        self.truecolor = command_line.truecolor.or_else(|| self.truecolor.take());
+        self.palette = command_line.palette.or_else(|| self.palette.take());
+        self.toggles.color_durations = command_line
+            .toggles
+            .color_durations
+            .or_else(|| self.toggles.color_durations.take());
+        self.toggles.color_terminated = command_line
+            .toggles
+            .color_terminated
+            .or_else(|| self.toggles.color_terminated.take());
+    }


a minor style suggestion, take it or leave it: what if, instead of mutating self, we had this function take self by value and return a new ViewOptiions? like this:

Suggested change

fn merge_with(&mut self, command_line: ViewOptions) {

self.no_colors = command_line.no_colors.or_else(|| self.no_colors.take());

self.lang = command_line.lang.or_else(|| self.lang.take());

self.ascii_only = command_line.ascii_only.or_else(|| self.ascii_only.take());

self.truecolor = command_line.truecolor.or_else(|| self.truecolor.take());

self.palette = command_line.palette.or_else(|| self.palette.take());

self.toggles.color_durations = command_line

.toggles

.color_durations

.or_else(|| self.toggles.color_durations.take());

self.toggles.color_terminated = command_line

.toggles

.color_terminated

.or_else(|| self.toggles.color_terminated.take());

}

fn merge_with(self, command_line: ViewOptions) -> ViewOptions {

Self {

no_colors: command_line.no_colors.or(self.no_colors),

lang: command_line.lang.or(self.lang),

ascii_only: command_line.ascii_only.or(self.ascii_only),

truecolor: command_line.truecolor.or(self.truecolor),

palette: command_line.palette.or(self.palette),

toggles: ColorToggles {

color_durations: command_line

.toggles

.color_durations

.or(self.toggles.color_durations)

color_terminated: command_line

.toggles

.color_terminated

.or(self.toggles.color_terminated)

}

}

}

This approach has one big advantage, in my opinion: if we add new fields to the ViewOptions type in the future, but forget to add code in merge_with to merge those fields, the current implementation will just use the values from the config file and ignore the values from the command-line, because it's modifying the existing instance of ViewOptions. If we make the change I suggested, though, forgetting to merge any new fields would result in a compile-time error, because we are constructing a new instance of ViewOptions, and we wouldn't be adding the new fields to the returned value. It would be nice to be able to prevent that kind of bug at compile-time, IMO.

(we probably want to do the same thing in the Config struct's merge_with function, too...)

This approach has one big advantage

Sounds great! I agree with your opinion.
682a261

hawkw · 2022-04-11T18:28:23Z

tokio-console/src/config.rs

+#[derive(Debug, Clone, Deserialize)]
+pub struct ConfigFile {
+    charset: Option<CharsetConfig>,
+    colors: Option<ColorsConfig>,
+}
+
+#[derive(Debug, Clone, Deserialize)]
+pub struct CharsetConfig {
+    lang: Option<String>,
+    ascii_only: Option<bool>,
+}
+
+#[derive(Debug, Clone, Deserialize)]
+pub struct ColorsConfig {
+    enabled: Option<bool>,
+    truecolor: Option<bool>,
+    palette: Option<Palette>,
+    enable: Option<ColorToggles>,


nit: these are only used for parsing the TOML config file, they're not exposed to the rest of the crate, as we turn them into a Config before returning it. i think these don't need to be pub:

Suggested change

#[derive(Debug, Clone, Deserialize)]

pub struct ConfigFile {

charset: Option<CharsetConfig>,

colors: Option<ColorsConfig>,

}

#[derive(Debug, Clone, Deserialize)]

pub struct CharsetConfig {

lang: Option<String>,

ascii_only: Option<bool>,

}

#[derive(Debug, Clone, Deserialize)]

pub struct ColorsConfig {

enabled: Option<bool>,

truecolor: Option<bool>,

palette: Option<Palette>,

enable: Option<ColorToggles>,

#[derive(Debug, Clone, Deserialize)]

struct ConfigFile {

charset: Option<CharsetConfig>,

colors: Option<ColorsConfig>,

}

#[derive(Debug, Clone, Deserialize)]

struct CharsetConfig {

lang: Option<String>,

ascii_only: Option<bool>,

}

#[derive(Debug, Clone, Deserialize)]

struct ColorsConfig {

enabled: Option<bool>,

truecolor: Option<bool>,

palette: Option<Palette>,

enable: Option<ColorToggles>,

also, it might be worth adding comments explaining that these types are used so that the structure of the config file can be different from how the CLI arguments are structured?

hawkw · 2022-04-11T18:29:49Z

tokio-console/src/config.rs

 }

 // === impl Config ===

 impl Config {
+    pub fn from_config() -> color_eyre::Result<Self> {


i'm not sure if from_config() is the best name for this, since it also parses the config from the command line arguments as well?

hawkw · 2022-04-11T18:31:57Z

tokio-console/src/config.rs

+        let lang = self.lang.as_deref().unwrap_or_default();
+        let ascii_only = self.ascii_only.unwrap_or(true);
+        lang.ends_with("UTF-8") && !ascii_only


nit: it occurs to me that we can simplify this a bit if we check the value of self.ascii_only before we touch self.lang:

Suggested change

let lang = self.lang.as_deref().unwrap_or_default();

let ascii_only = self.ascii_only.unwrap_or(true);

lang.ends_with("UTF-8") && !ascii_only

if !self.ascii_only.unwrap_or(true) {

return false;

}

self.lang.as_deref().unwrap_or_default().ends_with("UTF-8")

this has the advantage of avoiding the string comparison when the ascii_only flag is set.

hawkw · 2022-04-11T18:38:12Z

@nrskt:

I fixed the implementations and try to test again.

Thanks! Overall, this looks great!

I have questions about case7 and case8. please check it.

case7) Parse error

[Question] I want you to tell me how to output a better error message.

XDG_CONFIG/console.toml

[colors]
palette = "8"

./console.toml

[charset]
ascii_only = "string value"

❯ env -u LANG -u COLORTERM RUST_LOG=debug cargo run
    Finished dev [unoptimized + debuginfo] target(s) in 0.06s
     Running `target/debug/tokio-console`
failed to parse config file: Error {
    msg: "failed to parse Some(\"./console.toml\")",
    source: Error {
        inner: ErrorInner {
            kind: Custom,
            line: Some(
                1,
            ),
            col: 13,
            at: Some(
                23,
            ),
            message: "invalid type: string \"string value\", expected a boolean",
            key: [
                "charset",
                "ascii_only",
            ],
        },
    },
}

My suggestions #320 (comment) and #320 (comment) here explain how we can get a nicer error message for parse errors.

case8) Ignore unknown config field

[Question] Did you expect this behavior? I don't know how to check strictly.

XDG_CONFIG/console.toml

Hmm, I think we should probably emit some kind of error or warning on unknown fields... Looking at the toml crate, I notice that they do have an error variant for a struct containing unexpected fields: https://github.com/alexcrichton/toml-rs/blob/master/src/de.rs#L184-L192 so, it seems weird that it's not returning an error in this case. Could be a bug upstream?

nrskt · 2022-04-12T10:39:01Z

@hawkw

Thanks reviewing. I have made corrections to the areas you pointed out. Please review again.

Hmm, I think we should probably emit some kind of error or warning on unknown fields... Looking at the toml crate, I notice that they do have an error variant for a struct containing unexpected fields: https://github.com/alexcrichton/toml-rs/blob/master/src/de.rs#L184-L192 so, it seems weird that it's not returning an error in this case. Could be a bug upstream?

I'll investigate this one after tomorrow.

hawkw

Thanks reviewing. I have made corrections to the areas you pointed out. Please review again.

This looks great to me, and I'm excited to go ahead and merge it --- thanks for all your hard work on this, I'm really happy with how it's turned out.

Hmm, I think we should probably emit some kind of error or warning on unknown fields... Looking at the toml crate, I notice that they do have an error variant for a struct containing unexpected fields: https://github.com/alexcrichton/toml-rs/blob/master/src/de.rs#L184-L192 so, it seems weird that it's not returning an error in this case. Could be a bug upstream?

I'll investigate this one after tomorrow.

Since this is probably an issue with an upstream crate, I don't want to block merging this PR on it. Instead, we should just investigate whether it can be changed upstream, and then come back and make any additional changes as necessary. Thanks for looking into it!

hawkw · 2022-04-12T19:35:01Z

tokio-console/src/config.rs

 }

 // === impl Config ===

 impl Config {
+    /// Parse from config files and command line options.
+    pub fn parse() -> color_eyre::Result<Self> {


one last naming nit, take it or leave it: I'm not the biggest fan of calling this parse, as it conflicts with the Clap::parse implementation, which is a bit confusing. but, I can't immediately think of a better name for it, so 🤷‍♀️

hawkw · 2022-04-12T19:38:23Z

Once this is merged, I think we'll probably also want to add documentation about using the config file, but I'm happy to do that in a separate branch.

hawkw · 2022-04-12T19:49:02Z

I opened #323 for some potential follow-up work.

This branch builds upon the config file support added in #320 and adds a new `gen-config` subcommand to the `tokio-console` CLI. This command generates a new config TOML file with all config options populated with their default values (overridden by any CLI arguments passed to the console). This can be used when generating a new configuration file, so that the user can see all the default values. We can also use it to generate a config file for the documentation, which can be automatically kept in sync with the config file definintion in the app. This seems much nicer than hand-writing a config file for the docs, as the struct definitions in the `config` module serve as a single source of truth for the config file definition. ## Example usage: ```shell :; cargo run -- gen-config Finished dev [unoptimized + debuginfo] target(s) in 0.06s Running `target/debug/tokio-console gen-config` [charset] lang = 'en_US.UTF-8' ascii_only = false [colors] enabled = true truecolor = true palette = 'all' [colors.enable] durations = true terminated = true ```

…324) This branch builds upon the config file support added in #320 and adds a new `gen-config` subcommand to the `tokio-console` CLI. This command generates a new config TOML file with all config options populated with their default values (overridden by any CLI arguments passed to the console). This can be used when generating a new configuration file, so that the user can see all the default values. We can also use it to generate a config file for the documentation, which can be automatically kept in sync with the config file definintion in the app. This seems much nicer than hand-writing a config file for the docs, as the struct definitions in the `config` module serve as a single source of truth for the config file definition. ## Example usage: ```shell :; cargo run -- gen-config Finished dev [unoptimized + debuginfo] target(s) in 0.06s Running `target/debug/tokio-console gen-config` [charset] lang = 'en_US.UTF-8' ascii_only = false [colors] enabled = true truecolor = true palette = 'all' [colors.enable] durations = true terminated = true ```

<a name="0.1.4"></a> ## 0.1.4 (2022-04-13) #### Features * add autogenerated example config file to docs (#327) ([79da280](79da280)) * add `gen-config` subcommand to generate a config file (#324) ([e034f8d](e034f8d)) * surface dropped event count if there are any (#316) ([16df5d3](16df5d3)) * read configuration options from a config file (#320) ([defe346](defe346), closes [#310](310))

…ds (#330) This work is related to #320 (comment) return error message if the config file includes unknown fields. reference: https://serde.rs/container-attrs.html#deny_unknown_fields ## test 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 unknown = true # unknown field ``` ``` ❯ cargo run Finished dev [unoptimized + debuginfo] target(s) in 0.05s Running `target/debug/tokio-console` Error: failed to parse ./console.toml Caused by: unknown field `unknown`, expected `durations` or `terminated` for key `colors.enable` at line 10 column 1 Location: tokio-console/src/config.rs:388:14 ```

nrskt added 3 commits April 4, 2022 20:16

feat(console): add dependencies

cc335bf

feat(console): support config file

4c966b3

- fix command line parser - fix config file parser

feat(console): main function uses a new parser

74a7474

nrskt requested a review from a team as a code owner April 5, 2022 15:36

nrskt mentioned this pull request Apr 5, 2022

(WIP) Support configuration file #312

Closed

hawkw requested changes Apr 5, 2022

View reviewed changes

nrskt added 9 commits April 9, 2022 15:17

chore(console): move function to a mehtod.

3a16b75

chore(console): remove clone.

3599796

chore(console): refactor

99b5f70

feat(console): accept partial config file.

0847645

feat(console): add error handling for parsing config

222288a

chore(console): fix merge logic

1553aaa

chore(console): fix clippy warnings

88307e3

chore(console): print error messsage

0e6f9ba

chore(console): fix config field names

0a77d61

hawkw requested changes Apr 11, 2022

View reviewed changes

nrskt added 7 commits April 12, 2022 18:43

chore(console): improve error message

c87ddfc

chore(console): fix variant name

2da4fdf

chore(console): improve comments

10113fd

chore(console): fix ViewOptions::merge_with signature.

682a261

chore(console): fix visibility.

47ccf21

chore(console): improve method name

d950a98

chore(console): improve

e883cb6

hawkw approved these changes Apr 12, 2022

View reviewed changes

hawkw enabled auto-merge (squash) April 12, 2022 19:39

Merge branch 'main' into support_config_file

5ba68c3

hawkw mentioned this pull request Apr 12, 2022

console: enable and disable warnings from config files #323

Closed

hawkw merged commit defe346 into tokio-rs:main Apr 12, 2022

hawkw mentioned this pull request Apr 12, 2022

feat(console): add gen-config subcommand to generate a config file #324

Merged

hawkw mentioned this pull request Apr 13, 2022

console: prepare to release v0.1.3 #328

Merged

nrskt mentioned this pull request Apr 14, 2022

feat(console): parse error when use unknown fields #330

Merged

	let lang = self.lang.clone().unwrap_or_default();
	let lang = self.lang.as_ref().unwrap_or_default();

	.wrap_err_with(\|\| format!("failed to parse {:?}", path.into_path()))?;
	.wrap_err_with(\|\| format!("failed to parse {}", path.into_path().display()))?;

feat(console): Support config file #320

feat(console): Support config file #320

Conversation

nrskt commented Apr 5, 2022

Test

case1) no config files

case2) current config file

case3) XDG config file

case4) XDG config file and current config file

case5) XDG config file, current config file, and command-line option

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

nrskt Apr 9, 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

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 Apr 9, 2022

test

case1) no config files

case2) current config file

case3) XDG config file

case4) XDG config file and current config file

case5) XDG config file, current config file, and command-line option

Added

case6) XDG config file, current config file

case7) Parse error

case8) Ignore unknown config field

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

Choose a reason for hiding this comment

nrskt Apr 12, 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

Choose a reason for hiding this comment

hawkw commented Apr 11, 2022

case7) Parse error

case8) Ignore unknown config field

nrskt commented Apr 12, 2022

hawkw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw commented Apr 12, 2022

hawkw commented Apr 12, 2022

nrskt Apr 9, 2022 •

edited

Loading

nrskt Apr 12, 2022 •

edited

Loading