Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(WIP) Support configuration file #312

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 9 additions & 0 deletions console.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
no_colors = false
lang = "en_us.UTF-8"
ascii_only = true
palette = "8"
Comment on lines +1 to +4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



[toggles]
color_durations = true
color_terminated = true
Comment on lines +1 to +9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +1 to +9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds good, I'll try it.

2 changes: 2 additions & 0 deletions tokio-console/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,5 @@ h2 = "0.3"
regex = "1.5"
once_cell = "1.8"
humantime = "2.1.0"
serde = { version = "1", features = ["derive"] }
toml = "0.5"
48 changes: 45 additions & 3 deletions tokio-console/src/config.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
use crate::view::Palette;
use clap::{ArgGroup, Parser as Clap, ValueHint};
use serde::{Deserialize, Serialize};
use std::env;
use std::fs;
use std::path::Path;
use std::process::Command;
use std::str::FromStr;
use std::time::Duration;
Expand Down Expand Up @@ -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)]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pub(crate) view_options: ViewOptions,

/// How long to continue displaying completed tasks and dropped resources
Expand Down Expand Up @@ -66,7 +70,7 @@ pub struct Config {
#[derive(Debug)]
struct RetainFor(Option<Duration>);

#[derive(Clap, Debug, Clone)]
#[derive(Clap, Debug, Clone, Default, Deserialize, Serialize)]
#[clap(group = ArgGroup::new("colors").conflicts_with("no-colors"))]
pub struct ViewOptions {
/// Disable ANSI colors entirely.
Expand Down Expand Up @@ -107,7 +111,7 @@ pub struct ViewOptions {
}

/// Toggles on and off color coding for individual UI elements.
#[derive(Clap, Debug, Copy, Clone)]
#[derive(Clap, Debug, Copy, Clone, Deserialize, Serialize)]
pub struct ColorToggles {
/// Disable color-coding for duration units.
#[clap(long = "no-duration-colors", parse(from_flag = std::ops::Not::not), group = "colors")]
Expand All @@ -118,9 +122,37 @@ pub struct ColorToggles {
pub(crate) color_terminated: bool,
}

impl Default for ColorToggles {
fn default() -> Self {
Self {
color_durations: true,
color_terminated: true,
}
}
}

// === impl Config ===

impl Config {
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
}),
}
Comment on lines +147 to +153
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
Comment on lines +137 to +154
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried:

  • I don't know woking it on macOS or Windows.
  • Some rules are broken.
    • ex) it can use --palette and --colorterm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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/...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pub fn trace_init(&mut self) -> color_eyre::Result<()> {
let filter = std::mem::take(&mut self.env_filter);
use tracing_subscriber::prelude::*;
Expand Down Expand Up @@ -166,6 +198,16 @@ impl Config {
// === impl ViewOptions ===

impl ViewOptions {
pub(crate) fn from_config<P>(path: P) -> Option<Self>
where
P: AsRef<Path> + std::fmt::Debug,
{
match fs::read_to_string(&path) {
Err(_) => None,
Ok(conf) => toml::from_str(&conf).ok(),
}
}

pub fn is_utf8(&self) -> bool {
self.lang.ends_with("UTF-8") && !self.ascii_only
}
Expand Down
3 changes: 1 addition & 2 deletions tokio-console/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ use color_eyre::{eyre::eyre, Help, SectionExt};
use console_api::tasks::TaskDetails;
use state::State;

use clap::Parser as Clap;
use futures::stream::StreamExt;
use tokio::sync::{mpsc, watch};
use tui::{
Expand All @@ -26,7 +25,7 @@ mod warnings;

#[tokio::main]
async fn main() -> color_eyre::Result<()> {
let mut args = config::Config::parse();
let mut args = config::Config::from_config()?;
let retain_for = args.retain_for();
args.trace_init()?;
tracing::debug!(?args.target_addr, ?args.view_options);
Expand Down
9 changes: 8 additions & 1 deletion tokio-console/src/view/styles.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::config;
use serde::{Deserialize, Serialize};
use std::{borrow::Cow, str::FromStr};
use tui::{
style::{Color, Modifier, Style},
Expand All @@ -12,17 +13,23 @@ pub struct Styles {
pub(crate) utf8: bool,
}

#[derive(Debug, PartialEq, Eq, Copy, Clone)]
// possible_values = &["8", "16", "256", "all", "off"],
#[derive(Debug, PartialEq, Eq, Copy, Clone, Deserialize, Serialize)]
#[repr(u8)]
pub enum Palette {
#[serde(rename = "off")]
NoColors,
/// Use ANSI 8 color palette only.
#[serde(rename = "8")]
Ansi8,
/// Use ANSI 16 color palette only.
#[serde(rename = "16")]
Ansi16,
/// Enable ANSI 256-color palette.
#[serde(rename = "256")]
Ansi256,
/// Enable all RGB colors.
#[serde(rename = "all")]
All,
}

Expand Down