( )⚙ D10008 rust: Add config parsing support for more value types

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D10008

rust: Add config parsing support for more value types
ClosedPublic

Authored by SimonSapin on Feb 17 2021, 7:58 AM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG305d74c262de: rust: Add config parsing support for more value types
rHG87e7fe5bce09: rust: Add config parsing support for more value types
Summary
  • Rust str (ASCII or UTF-8)
  • Integer
  • Byte quantities

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

SimonSapin created this revision.Feb 17 2021, 7:58 AM
Alphare accepted this revision.Feb 18 2021, 4:33 AM
Alphare added a subscriber: Alphare.
Alphare added inline comments.
rust/hg-core/src/config/config.rs
261

Should private methods be under public ones for readability?

SimonSapin added inline comments.Feb 19 2021, 3:07 AM
rust/hg-core/src/config/config.rs
261

My thinking was to have this method near related methods (that call it). We could also separate private v.s. public, but isn’t rustdoc better than looking a source directly for a reader who’s interested in public APIs more than their implementation?

Alphare added inline comments.Feb 19 2021, 3:52 AM
rust/hg-core/src/config/config.rs
261

Either way is fine with me. I usually regroup it the same way you do, but I was asked last time to split them in public then private. Let's get democratic and see if anyone cares enough ;)

SimonSapin added a commit: rHG87e7fe5bce09: rust: Add config parsing support for more value types.Feb 24 2021, 11:09 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG87e7fe5bce09: rust: Add config parsing support for more value types (authored by SimonSapin). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mrust/hg-core/src/config/config.rs (94 lines)

Diff 25645

rust/hg-core/src/config/config.rs

use super::layer; use super::layer;
use crate::config::layer::{ use crate::config::layer::{
ConfigError, ConfigLayer, ConfigParseError, ConfigValue, ConfigError, ConfigLayer, ConfigParseError, ConfigValue,
}; };
use crate::utils::files::get_bytes_from_os_str; use crate::utils::files::get_bytes_from_os_str;
use format_bytes::{write_bytes, DisplayBytes}; use format_bytes::{write_bytes, DisplayBytes};
use std::env; use std::env;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
use std::str;
use crate::errors::{HgResultExt, IoResultExt}; use crate::errors::{HgResultExt, IoResultExt};
/// Holds the config values for the current repository /// Holds the config values for the current repository
/// TODO update this docstring once we support more sources /// TODO update this docstring once we support more sources
pub struct Config { pub struct Config {
layers: Vec<layer::ConfigLayer>, layers: Vec<layer::ConfigLayer>,
} }
pub fn parse_bool(v: &[u8]) -> Option<bool> { pub fn parse_bool(v: &[u8]) -> Option<bool> {
match v.to_ascii_lowercase().as_slice() { match v.to_ascii_lowercase().as_slice() {
b"1" | b"yes" | b"true" | b"on" | b"always" => Some(true), b"1" | b"yes" | b"true" | b"on" | b"always" => Some(true),
b"0" | b"no" | b"false" | b"off" | b"never" => Some(false), b"0" | b"no" | b"false" | b"off" | b"never" => Some(false),
_ => None, _ => None,
} }
} }
pub fn parse_byte_size(value: &[u8]) -> Option<u64> {
let value = str::from_utf8(value).ok()?.to_ascii_lowercase();
const UNITS: &[(&str, u64)] = &[
("g", 1 << 30),
("gb", 1 << 30),
("m", 1 << 20),
("mb", 1 << 20),
("k", 1 << 10),
("kb", 1 << 10),
("b", 1 << 0), // Needs to be last
];
for &(unit, multiplier) in UNITS {
// TODO: use `value.strip_suffix(unit)` when we require Rust 1.45+
if value.ends_with(unit) {
let value_before_unit = &value[..value.len() - unit.len()];
let float: f64 = value_before_unit.trim().parse().ok()?;
if float >= 0.0 {
return Some((float * multiplier as f64).round() as u64);
} else {
return None;
}
}
}
value.parse().ok()
}
impl Config { impl Config {
/// Load system and user configuration from various files. /// Load system and user configuration from various files.
/// ///
/// This is also affected by some environment variables. /// This is also affected by some environment variables.
pub fn load( pub fn load(
cli_config_args: impl IntoIterator<Item = impl AsRef<[u8]>>, cli_config_args: impl IntoIterator<Item = impl AsRef<[u8]>>,
) -> Result<Self, ConfigError> { ) -> Result<Self, ConfigError> {
let mut config = Self { layers: Vec::new() }; let mut config = Self { layers: Vec::new() };
// TODO: check if this file should be trusted: // TODO: check if this file should be trusted:
// `mercurial/ui.py:427` // `mercurial/ui.py:427`
repo_config.add_trusted_file(path)?; repo_config.add_trusted_file(path)?;
} }
repo_config.layers.extend(cli_layers); repo_config.layers.extend(cli_layers);
Ok(repo_config) Ok(repo_config)
} }
/// Returns an `Err` if the first value found is not a valid boolean. fn get_parse<'config, T: 'config>(
AlphareUnsubmitted
Not Done

Should private methods be under public ones for readability?

Alphare: Should private methods be under public ones for readability?
SimonSapinAuthorUnsubmitted
Done

My thinking was to have this method near related methods (that call it). We could also separate private v.s. public, but isn’t rustdoc better than looking a source directly for a reader who’s interested in public APIs more than their implementation?

SimonSapin: My thinking was to have this method near related methods (that call it). We could also separate…
AlphareUnsubmitted
Not Done

Either way is fine with me. I usually regroup it the same way you do, but I was asked last time to split them in public then private. Let's get democratic and see if anyone cares enough ;)

Alphare: Either way is fine with me. I usually regroup it the same way you do, but I was asked last time…
/// Otherwise, returns an `Ok(option)`, where `option` is the boolean if &'config self,
/// found, or `None`.
pub fn get_option(
&self,
section: &[u8], section: &[u8],
item: &[u8], item: &[u8],
) -> Result<Option<bool>, ConfigParseError> { parse: impl Fn(&'config [u8]) -> Option<T>,
) -> Result<Option<T>, ConfigParseError> {
match self.get_inner(&section, &item) { match self.get_inner(&section, &item) {
Some((layer, v)) => match parse_bool(&v.bytes) { Some((layer, v)) => match parse(&v.bytes) {
Some(b) => Ok(Some(b)), Some(b) => Ok(Some(b)),
None => Err(ConfigParseError { None => Err(ConfigParseError {
origin: layer.origin.to_owned(), origin: layer.origin.to_owned(),
line: v.line, line: v.line,
bytes: v.bytes.to_owned(), bytes: v.bytes.to_owned(),
}), }),
}, },
None => Ok(None), None => Ok(None),
} }
} }
/// Returns an `Err` if the first value found is not a valid UTF-8 string.
/// Otherwise, returns an `Ok(value)` if found, or `None`.
pub fn get_str(
&self,
section: &[u8],
item: &[u8],
) -> Result<Option<&str>, ConfigParseError> {
self.get_parse(section, item, |value| str::from_utf8(value).ok())
}
/// Returns an `Err` if the first value found is not a valid unsigned
/// integer. Otherwise, returns an `Ok(value)` if found, or `None`.
pub fn get_u32(
&self,
section: &[u8],
item: &[u8],
) -> Result<Option<u32>, ConfigParseError> {
self.get_parse(section, item, |value| {
str::from_utf8(value).ok()?.parse().ok()
})
}
/// Returns an `Err` if the first value found is not a valid file size
/// value such as `30` (default unit is bytes), `7 MB`, or `42.5 kb`.
/// Otherwise, returns an `Ok(value_in_bytes)` if found, or `None`.
pub fn get_byte_size(
&self,
section: &[u8],
item: &[u8],
) -> Result<Option<u64>, ConfigParseError> {
self.get_parse(section, item, parse_byte_size)
}
/// Returns an `Err` if the first value found is not a valid boolean.
/// Otherwise, returns an `Ok(option)`, where `option` is the boolean if
/// found, or `None`.
pub fn get_option(
&self,
section: &[u8],
item: &[u8],
) -> Result<Option<bool>, ConfigParseError> {
self.get_parse(section, item, parse_bool)
}
/// Returns the corresponding boolean in the config. Returns `Ok(false)` /// Returns the corresponding boolean in the config. Returns `Ok(false)`
/// if the value is not found, an `Err` if it's not a valid boolean. /// if the value is not found, an `Err` if it's not a valid boolean.
pub fn get_bool( pub fn get_bool(
&self, &self,
section: &[u8], section: &[u8],
item: &[u8], item: &[u8],
) -> Result<bool, ConfigError> { ) -> Result<bool, ConfigError> {
Ok(self.get_option(section, item)?.unwrap_or(false)) Ok(self.get_option(section, item)?.unwrap_or(false))
let tmpdir_path = tmpdir.path(); let tmpdir_path = tmpdir.path();
let mut included_file = let mut included_file =
File::create(&tmpdir_path.join("included.rc")).unwrap(); File::create(&tmpdir_path.join("included.rc")).unwrap();
included_file.write_all(b"[section]\nitem=value1").unwrap(); included_file.write_all(b"[section]\nitem=value1").unwrap();
let base_config_path = tmpdir_path.join("base.rc"); let base_config_path = tmpdir_path.join("base.rc");
let mut config_file = File::create(&base_config_path).unwrap(); let mut config_file = File::create(&base_config_path).unwrap();
let data = let data =
b"[section]\nitem=value0\n%include included.rc\nitem=value2"; b"[section]\nitem=value0\n%include included.rc\nitem=value2\n\
[section2]\ncount = 4\nsize = 1.5 KB\nnot-count = 1.5\nnot-size = 1 ub";
config_file.write_all(data).unwrap(); config_file.write_all(data).unwrap();
let sources = vec![ConfigSource::AbsPath(base_config_path)]; let sources = vec![ConfigSource::AbsPath(base_config_path)];
let config = Config::load_from_explicit_sources(sources) let config = Config::load_from_explicit_sources(sources)
.expect("expected valid config"); .expect("expected valid config");
let (_, value) = config.get_inner(b"section", b"item").unwrap(); let (_, value) = config.get_inner(b"section", b"item").unwrap();
assert_eq!( assert_eq!(
value, value,
&ConfigValue { &ConfigValue {
bytes: b"value2".to_vec(), bytes: b"value2".to_vec(),
line: Some(4) line: Some(4)
} }
); );
let value = config.get(b"section", b"item").unwrap(); let value = config.get(b"section", b"item").unwrap();
assert_eq!(value, b"value2",); assert_eq!(value, b"value2",);
assert_eq!( assert_eq!(
config.get_all(b"section", b"item"), config.get_all(b"section", b"item"),
[b"value2", b"value1", b"value0"] [b"value2", b"value1", b"value0"]
); );
assert_eq!(config.get_u32(b"section2", b"count").unwrap(), Some(4));
assert_eq!(
config.get_byte_size(b"section2", b"size").unwrap(),
Some(1024 + 512)
);
assert!(config.get_u32(b"section2", b"not-count").is_err());
assert!(config.get_byte_size(b"section2", b"not-size").is_err());
} }
} }