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.

Sign up for GitHub

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

config: migrate underlying data structure to toml_edit #5060

Merged
merged 3 commits into from
Dec 10, 2024
Merged

config: migrate underlying data structure to toml_edit #5060

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4888641
config: migrate underlying data structure to toml_edit
yuja Nov 29, 2024
8080981
config: preserve definition order of table items
yuja Nov 29, 2024
061adac
formatter: in tests, use IndexMap to preserve definition order of colors
yuja Dec 1, 2024
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

### Breaking changes

* Configuration variables are no longer "stringly" typed. For example, `true` is
not converted to a string `"true"`, and vice versa.
yuja marked this conversation as resolved.
Show resolved Hide resolved

* The following configuration variables are now parsed strictly:
`ui.progress-indicator`, `ui.quiet`

Expand Down
13 changes: 5 additions & 8 deletions cli/src/cli_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2694,22 +2694,19 @@ fn load_template_aliases(
Ok(Some(table)) => table,
Ok(None) => continue,
Err(item) => {
// TODO: rewrite error construction after migrating to toml_edit
let error = item.clone().into_table().unwrap_err();
return Err(ConfigGetError::Type {
name: table_name.to_string(),
error: error.into(),
error: format!("Expected a table, but is {}", item.type_name()).into(),
source_path: layer.path.clone(),
}
.into());
}
};
// TODO: remove sorting after migrating to toml_edit
for (decl, value) in table.iter().sorted_by_key(|&(decl, _)| decl) {
let r = value
.clone()
.into_string()
.map_err(|e| e.to_string())
for (decl, item) in table.iter().sorted_by_key(|&(decl, _)| decl) {
let r = item
.as_str()
.ok_or_else(|| format!("Expected a string, but is {}", item.type_name()))
.and_then(|v| aliases_map.insert(decl, v).map_err(|e| e.to_string()));
if let Err(s) = r {
writeln!(
Expand Down
35 changes: 18 additions & 17 deletions cli/src/commands/config/get.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,10 @@
// limitations under the License.

use std::io::Write as _;
use std::path::PathBuf;

use clap_complete::ArgValueCandidates;
use jj_lib::config::ConfigError;
use jj_lib::config::ConfigGetError;
use jj_lib::config::ConfigNamePathBuf;
use jj_lib::config::ConfigValue;
use tracing::instrument;

use crate::cli_util::CommandHelper;
Expand Down Expand Up @@ -48,21 +46,24 @@ pub fn cmd_config_get(
command: &CommandHelper,
args: &ConfigGetArgs,
) -> Result<(), CommandError> {
let value = command.settings().get_value(&args.name)?;
let stringified = value.into_string().map_err(|err| -> CommandError {
match err {
ConfigError::Type {
origin, unexpected, ..
} => ConfigGetError::Type {
name: args.name.to_string(),
error: format!("Expected a value convertible to a string, but is {unexpected}")
.into(),
source_path: origin.map(PathBuf::from),
let stringified = command
.settings()
.get_value_with(&args.name, |value| match value {
// Remove extra formatting from a string value
ConfigValue::String(v) => Ok(v.into_value()),
// Print other values in TOML syntax (but whitespace trimmed)
ConfigValue::Integer(_)
| ConfigValue::Float(_)
| ConfigValue::Boolean(_)
| ConfigValue::Datetime(_) => Ok(value.decorated("", "").to_string()),
// TODO: maybe okay to just print array or table in TOML syntax?
ConfigValue::Array(_) => {
Err("Expected a value convertible to a string, but is an array")
}
.into(),
err => err.into(),
}
})?;
ConfigValue::InlineTable(_) => {
Err("Expected a value convertible to a string, but is a table")
}
})?;
writeln!(ui.stdout(), "{stringified}")?;
Ok(())
}
4 changes: 2 additions & 2 deletions cli/src/commands/config/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ use crate::cli_util::CommandHelper;
use crate::command_error::CommandError;
use crate::complete;
use crate::config::resolved_config_values;
use crate::config::to_toml_value;
use crate::config::AnnotatedValue;
use crate::generic_templater::GenericTemplateLanguage;
use crate::template_builder::TemplateLanguage as _;
Expand Down Expand Up @@ -122,8 +121,9 @@ fn config_template_language() -> GenericTemplateLanguage<'static, AnnotatedValue
});
language.add_keyword("value", |self_property| {
// TODO: would be nice if we can provide raw dynamically-typed value
// .decorated("", "") to trim leading/trailing whitespace
let out_property =
self_property.and_then(|annotated| Ok(to_toml_value(&annotated.value)?.to_string()));
self_property.map(|annotated| annotated.value.decorated("", "").to_string());
Ok(L::wrap_string(out_property))
});
language.add_keyword("overridden", |self_property| {
Expand Down
6 changes: 4 additions & 2 deletions cli/src/commands/fix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ use crate::command_error::config_error;
use crate::command_error::print_parse_diagnostics;
use crate::command_error::CommandError;
use crate::complete;
use crate::config::to_toml_value;
use crate::config::CommandNameAndArgs;
use crate::ui::Ui;

Expand Down Expand Up @@ -470,7 +469,10 @@ fn get_tools_config(ui: &mut Ui, settings: &UserSettings) -> Result<ToolsConfig,
command = {}
patterns = ["all()"]
"###,
to_toml_value(&settings.get_value("fix.tool-command").unwrap()).unwrap()
settings
.get_value("fix.tool-command")
.unwrap()
.decorated("", "") // trim whitespace
)?;
}
let tools: Vec<ToolConfig> = settings
Expand Down
131 changes: 58 additions & 73 deletions cli/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,32 +51,6 @@ pub fn parse_toml_value_or_bare_string(value_str: &str) -> toml_edit::Value {
}
}

pub fn to_toml_value(value: &ConfigValue) -> Result<toml_edit::Value, ConfigError> {
fn type_error<T: fmt::Display>(message: T) -> ConfigError {
ConfigError::Message(message.to_string())
}
// It's unlikely that the config object contained unsupported values, but
// there's no guarantee. For example, values coming from environment
// variables might be big int.
match value.kind {
config::ValueKind::Nil => Err(type_error(format!("Unexpected value: {value}"))),
config::ValueKind::Boolean(v) => Ok(v.into()),
config::ValueKind::I64(v) => Ok(v.into()),
config::ValueKind::I128(v) => Ok(i64::try_from(v).map_err(type_error)?.into()),
config::ValueKind::U64(v) => Ok(i64::try_from(v).map_err(type_error)?.into()),
config::ValueKind::U128(v) => Ok(i64::try_from(v).map_err(type_error)?.into()),
config::ValueKind::Float(v) => Ok(v.into()),
config::ValueKind::String(ref v) => Ok(v.into()),
// TODO: Remove sorting when config crate maintains deterministic ordering.
config::ValueKind::Table(ref table) => table
.iter()
.sorted_by_key(|(k, _)| *k)
.map(|(k, v)| Ok((k, to_toml_value(v)?)))
.collect(),
config::ValueKind::Array(ref array) => array.iter().map(to_toml_value).collect(),
}
}

#[derive(Error, Debug)]
pub enum ConfigEnvError {
#[error(transparent)]
Expand Down Expand Up @@ -118,24 +92,30 @@ pub fn resolved_config_values(
};
let mut config_stack = vec![(filter_prefix.clone(), top_item)];
while let Some((name, item)) = config_stack.pop() {
match &item.kind {
config::ValueKind::Table(table) => {
// TODO: Remove sorting when config crate maintains deterministic ordering.
for (k, v) in table.iter().sorted_by_key(|(k, _)| *k).rev() {
let mut sub_name = name.clone();
sub_name.push(k);
config_stack.push((sub_name, v));
}
}
_ => {
config_vals.push(AnnotatedValue {
name,
value: item.to_owned(),
source: layer.source,
// Note: Value updated below.
is_overridden: false,
});
// TODO: item.as_table() to print inline table as a value?
if let Some(table) = item.as_table_like() {
// table.iter() does not implement DoubleEndedIterator as of
// toml_edit 0.22.22.
let frame = config_stack.len();
// TODO: Remove sorting
for (k, v) in table.iter().sorted_by_key(|(k, _)| *k) {
let mut sub_name = name.clone();
sub_name.push(k);
config_stack.push((sub_name, v));
}
config_stack[frame..].reverse();
} else {
let value = item
.clone()
.into_value()
.expect("Item::None should not exist in table");
config_vals.push(AnnotatedValue {
name,
value,
source: layer.source,
// Note: Value updated below.
is_overridden: false,
});
}
}
}
Expand Down Expand Up @@ -798,12 +778,13 @@ mod tests {
},
],
),
value: Value {
origin: None,
kind: String(
"[email protected]",
),
},
value: String(
Formatted {
value: "[email protected]",
repr: "default",
decor: Decor { .. },
},
),
source: EnvBase,
is_overridden: true,
},
Expand All @@ -824,12 +805,13 @@ mod tests {
},
],
),
value: Value {
origin: None,
kind: String(
"base-user-name",
),
},
value: String(
Formatted {
value: "base-user-name",
repr: "default",
decor: Decor { .. },
},
),
source: EnvBase,
is_overridden: false,
},
Expand All @@ -850,12 +832,13 @@ mod tests {
},
],
),
value: Value {
origin: None,
kind: String(
"[email protected]",
),
},
value: String(
Formatted {
value: "[email protected]",
repr: "default",
decor: Decor { .. },
},
),
source: Repo,
is_overridden: false,
},
Expand Down Expand Up @@ -897,12 +880,13 @@ mod tests {
},
],
),
value: Value {
origin: None,
kind: String(
"user-FOO",
),
},
value: String(
Formatted {
value: "user-FOO",
repr: "default",
decor: Decor { .. },
},
),
source: User,
is_overridden: false,
},
Expand All @@ -923,12 +907,13 @@ mod tests {
},
],
),
value: Value {
origin: None,
kind: String(
"repo-BAR",
),
},
value: String(
Formatted {
value: "repo-BAR",
repr: "default",
decor: Decor { .. },
},
),
source: Repo,
is_overridden: false,
},
Expand Down
61 changes: 29 additions & 32 deletions cli/src/formatter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,44 +417,41 @@ fn rules_from_config(config: &StackedConfig) -> Result<Rules, ConfigGetError> {
.split_whitespace()
.map(ToString::to_string)
.collect_vec();
// TODO: report invalid type?
let value = config.get_value(["colors", key])?;
match value.kind {
config::ValueKind::String(color_name) => {
let style = Style {
fg_color: Some(color_for_name_or_hex(&color_name).map_err(to_config_err)?),
bg_color: None,
bold: None,
underlined: None,
};
result.push((labels, style));
}
config::ValueKind::Table(style_table) => {
let mut style = Style::default();
if let Some(value) = style_table.get("fg") {
if let config::ValueKind::String(color_name) = &value.kind {
style.fg_color =
Some(color_for_name_or_hex(color_name).map_err(to_config_err)?);
}
if let Some(color_name) = value.as_str() {
let style = Style {
fg_color: Some(color_for_name_or_hex(color_name).map_err(to_config_err)?),
bg_color: None,
bold: None,
underlined: None,
};
result.push((labels, style));
} else if let Some(style_table) = value.as_inline_table() {
let mut style = Style::default();
if let Some(item) = style_table.get("fg") {
if let Some(color_name) = item.as_str() {
style.fg_color =
Some(color_for_name_or_hex(color_name).map_err(to_config_err)?);
}
if let Some(value) = style_table.get("bg") {
if let config::ValueKind::String(color_name) = &value.kind {
style.bg_color =
Some(color_for_name_or_hex(color_name).map_err(to_config_err)?);
}
}
if let Some(item) = style_table.get("bg") {
if let Some(color_name) = item.as_str() {
style.bg_color =
Some(color_for_name_or_hex(color_name).map_err(to_config_err)?);
}
if let Some(value) = style_table.get("bold") {
if let config::ValueKind::Boolean(value) = &value.kind {
style.bold = Some(*value);
}
}
if let Some(item) = style_table.get("bold") {
if let Some(value) = item.as_bool() {
style.bold = Some(value);
}
if let Some(value) = style_table.get("underline") {
if let config::ValueKind::Boolean(value) = &value.kind {
style.underlined = Some(*value);
}
}
if let Some(item) = style_table.get("underline") {
if let Some(value) = item.as_bool() {
style.underlined = Some(value);
}
result.push((labels, style));
}
_ => {}
result.push((labels, style));
}
}
Ok(result)
Expand Down
Loading