From cb18dd5200df36107bf1cc23b7f3fb7a3c4dd79a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Richter?= <2268851+x3rAx@users.noreply.github.com> Date: Sat, 13 Aug 2022 04:13:50 +0200 Subject: [PATCH] Add decimals to int when using `into string --decimals` (#6085) * Add decimals to int when using `into string --decimals` * Add tests for `into string` when converting int with `--decimals` * Apply formatting * Merge `into_str` test files * Comment out unused code and add TODOs * Use decimal separator depending on system locale * Add test helper to run closure in different locale * Add tests for int-to-string conversion using different locales * Add utils function to get system locale * Add panic message when locking mutex fails * Catch and resume panic later to prevent Mutex poisoning when test fails * Move test to `nu-test-support` to keep `nu-utils` free of `nu-*` dependencies See https://github.com/nushell/nushell/pull/6085#issuecomment-1193131694 * Rename test support fn `with_fake_locale` to `with_locale_override` * Move `get_system_locale()` to `locale` module * Allow overriding locale with special env variable (when not in release) * Use special env var to override locale during testing * Allow callback to return a value in `with_locale_override()` * Allow multiple options in `nu!` macro * Allow to set locale as `nu!` macro option * Use new `locale` option of `nu!` macro instead of `with_locale_override` Using the `locale` options does not lock the `LOCALE_OVERRIDE_MUTEX` mutex in `nu-test-support::locale_override` but instead calls the `nu` command directly with the `NU_LOCALE_OVERRIDE` environment variable. This allows for parallel test excecution. * Fix: Add option identifier for `cwd` in usage of `nu!` macro * Rely on `Display` trait for formatting `nu!` macro command - Removed the `DisplayPath` trait - Implement `Display` for `AbsolutePath`, `RelativePath` and `AbsoluteFile` * Default to locale `en_US.UTF-8` for tests when using `nu!` macro * Add doc comment to `nu!` macro * Format code using `cargo fmt --all` * Pass function directly instead of wrapping the call in a closure https://rust-lang.github.io/rust-clippy/master/index.html#redundant_closure * Pass function to `or_else()` instead of calling it inside `or()` https://rust-lang.github.io/rust-clippy/master/index.html#or_fun_call * Fix: Add option identifier for `cwd` in usage of `nu!` macro --- Cargo.lock | 6 + crates/nu-command/Cargo.toml | 1 + .../nu-command/src/conversions/into/string.rs | 57 +++---- crates/nu-command/tests/commands/cd.rs | 6 +- crates/nu-command/tests/commands/cp.rs | 12 +- crates/nu-command/tests/commands/ls.rs | 2 +- crates/nu-command/tests/commands/move_/mv.rs | 2 +- crates/nu-command/tests/commands/source.rs | 4 +- .../tests/commands/str_/into_string.rs | 84 ++++++++++ crates/nu-command/tests/commands/use_.rs | 6 +- crates/nu-protocol/src/value/mod.rs | 24 +-- crates/nu-test-support/Cargo.toml | 3 + crates/nu-test-support/src/fs.rs | 39 ++--- crates/nu-test-support/src/lib.rs | 1 + crates/nu-test-support/src/locale_override.rs | 46 ++++++ crates/nu-test-support/src/macros.rs | 153 +++++++++++++++--- .../tests/get_system_locale.rs | 19 +++ crates/nu-utils/Cargo.toml | 2 + crates/nu-utils/src/lib.rs | 2 + crates/nu-utils/src/locale.rs | 39 +++++ tests/shell/environment/mod.rs | 4 +- 21 files changed, 390 insertions(+), 122 deletions(-) create mode 100644 crates/nu-test-support/src/locale_override.rs create mode 100644 crates/nu-test-support/tests/get_system_locale.rs create mode 100644 crates/nu-utils/src/locale.rs diff --git a/Cargo.lock b/Cargo.lock index bf0208c9f4..0b63c04256 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2636,6 +2636,7 @@ dependencies = [ "nu-test-support", "nu-utils", "num 0.4.0", + "num-format", "num-traits", "pathdiff", "polars", @@ -2814,9 +2815,12 @@ version = "0.66.4" dependencies = [ "getset", "hamcrest2", + "lazy_static", "nu-glob", "nu-path", + "nu-utils", "num-bigint 0.4.3", + "num-format", "tempfile", ] @@ -2826,6 +2830,8 @@ version = "0.66.4" dependencies = [ "crossterm_winapi", "lscolors", + "num-format", + "sys-locale", ] [[package]] diff --git a/crates/nu-command/Cargo.toml b/crates/nu-command/Cargo.toml index ec3890edb8..b2405cfe1c 100644 --- a/crates/nu-command/Cargo.toml +++ b/crates/nu-command/Cargo.toml @@ -24,6 +24,7 @@ nu-term-grid = { path = "../nu-term-grid", version = "0.66.4" } nu-test-support = { path = "../nu-test-support", version = "0.66.4" } nu-utils = { path = "../nu-utils", version = "0.66.4" } nu-ansi-term = "0.46.0" +num-format = { version = "0.4.0" } # Potential dependencies for extras alphanumeric-sort = "1.4.4" diff --git a/crates/nu-command/src/conversions/into/string.rs b/crates/nu-command/src/conversions/into/string.rs index ad6da56514..387933bbbc 100644 --- a/crates/nu-command/src/conversions/into/string.rs +++ b/crates/nu-command/src/conversions/into/string.rs @@ -5,8 +5,8 @@ use nu_protocol::{ into_code, Category, Config, Example, IntoPipelineData, PipelineData, ShellError, Signature, Span, SyntaxShape, Value, }; - -// TODO num_format::SystemLocale once platform-specific dependencies are stable (see Cargo.toml) +use nu_utils::get_system_locale; +use num_format::ToFormattedString; #[derive(Clone)] pub struct SubCommand; @@ -216,21 +216,8 @@ pub fn action( ) -> Value { match input { Value::Int { val, .. } => { - let res = if group_digits { - format_int(*val) // int.to_formatted_string(*locale) - } else if let Some(dig) = digits { - let mut val_with_trailing_zeroes = val.to_string(); - if dig != 0 { - val_with_trailing_zeroes.push('.'); - } - for _ in 0..dig { - val_with_trailing_zeroes.push('0'); - } - val_with_trailing_zeroes - } else { - val.to_string() - }; - + let decimal_value = digits.unwrap_or(0) as usize; + let res = format_int(*val, group_digits, decimal_value); Value::String { val: res, span } } Value::Float { val, .. } => { @@ -305,21 +292,29 @@ pub fn action( }, } } -fn format_int(int: i64) -> String { - int.to_string() - // TODO once platform-specific dependencies are stable (see Cargo.toml) - // #[cfg(windows)] - // { - // int.to_formatted_string(&Locale::en) - // } - // #[cfg(not(windows))] - // { - // match SystemLocale::default() { - // Ok(locale) => int.to_formatted_string(&locale), - // Err(_) => int.to_formatted_string(&Locale::en), - // } - // } +fn format_int(int: i64, group_digits: bool, decimals: usize) -> String { + let locale = get_system_locale(); + + let str = if group_digits { + int.to_formatted_string(&locale) + } else { + int.to_string() + }; + + if decimals > 0 { + let decimal_point = locale.decimal(); + + format!( + "{}{decimal_point}{dummy:0 { - let locale_string = get_locale().unwrap_or_else(|| String::from("en-US")); - // Since get_locale() and Locale::from_name() don't always return the same items - // we need to try and parse it to match. For instance, a valid locale is de_DE - // however Locale::from_name() wants only de so we split and parse it out. - let locale_string = locale_string.replace('_', "-"); // en_AU -> en-AU - let locale = match Locale::from_name(&locale_string) { - Ok(loc) => loc, - _ => { - let all = num_format::Locale::available_names(); - let locale_prefix = &locale_string.split('-').collect::>(); - if all.contains(&locale_prefix[0]) { - // eprintln!("Found alternate: {}", &locale_prefix[0]); - Locale::from_name(locale_prefix[0]).unwrap_or(Locale::en) - } else { - // eprintln!("Unable to find matching locale. Defaulting to en-US"); - Locale::en - } - } - }; + let locale = get_system_locale(); let locale_byte = adj_byte.get_value() as u64; let locale_byte_string = locale_byte.to_formatted_string(&locale); diff --git a/crates/nu-test-support/Cargo.toml b/crates/nu-test-support/Cargo.toml index e89fb2db3c..ffad69f5eb 100644 --- a/crates/nu-test-support/Cargo.toml +++ b/crates/nu-test-support/Cargo.toml @@ -12,6 +12,9 @@ doctest = false [dependencies] nu-path = { path="../nu-path", version = "0.66.4" } nu-glob = { path = "../nu-glob", version = "0.66.4" } +nu-utils = { path="../nu-utils", version = "0.66.4" } +lazy_static = "1.4.0" +num-format = "0.4.0" getset = "0.1.1" num-bigint = { version="0.4.3", features=["serde"] } diff --git a/crates/nu-test-support/src/fs.rs b/crates/nu-test-support/src/fs.rs index 909e2ebd77..be6b5c2bf6 100644 --- a/crates/nu-test-support/src/fs.rs +++ b/crates/nu-test-support/src/fs.rs @@ -1,3 +1,4 @@ +use std::fmt::Display; use std::io::Read; use std::ops::Div; use std::path::{Path, PathBuf}; @@ -113,45 +114,25 @@ impl> Div for &RelativePath { RelativePath::new(result) } } -pub trait DisplayPath { - fn display_path(&self) -> String; -} -impl DisplayPath for AbsolutePath { - fn display_path(&self) -> String { - self.inner.display().to_string() +impl Display for AbsoluteFile { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.inner.display()) } } -impl DisplayPath for PathBuf { - fn display_path(&self) -> String { - self.display().to_string() +impl Display for AbsolutePath { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.inner.display()) } } -impl DisplayPath for str { - fn display_path(&self) -> String { - self.to_string() +impl Display for RelativePath { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.inner.display()) } } -impl DisplayPath for &str { - fn display_path(&self) -> String { - (*self).to_string() - } -} - -impl DisplayPath for String { - fn display_path(&self) -> String { - self.clone() - } -} - -impl DisplayPath for &String { - fn display_path(&self) -> String { - (*self).to_string() - } -} pub enum Stub<'a> { FileWithContent(&'a str, &'a str), FileWithContentToBeTrimmed(&'a str, &'a str), diff --git a/crates/nu-test-support/src/lib.rs b/crates/nu-test-support/src/lib.rs index 78ff10b326..2dfd12824a 100644 --- a/crates/nu-test-support/src/lib.rs +++ b/crates/nu-test-support/src/lib.rs @@ -1,5 +1,6 @@ pub mod commands; pub mod fs; +pub mod locale_override; pub mod macros; pub mod playground; diff --git a/crates/nu-test-support/src/locale_override.rs b/crates/nu-test-support/src/locale_override.rs new file mode 100644 index 0000000000..953c58063a --- /dev/null +++ b/crates/nu-test-support/src/locale_override.rs @@ -0,0 +1,46 @@ +#![cfg(debug_assertions)] + +use std::sync::{Arc, Mutex}; + +use lazy_static::lazy_static; +use nu_utils::locale::LOCALE_OVERRIDE_ENV_VAR; + +lazy_static! { + static ref LOCALE_OVERRIDE_MUTEX: Arc> = Arc::new(Mutex::new(())); +} + +/// Run a closure in a fake locale environment. +/// +/// Before the closure is executed, an environment variable whose name is +/// defined in `nu_utils::locale::LOCALE_OVERRIDE_ENV_VAR` is set to the value +/// provided by `locale_string`. When the closure is done, the previous value is +/// restored. +/// +/// Environment variables are global values. So when they are changed by one +/// thread they are changed for all others. To prevent a test from overwriting +/// the environment variable of another test, a mutex is used. +pub fn with_locale_override(locale_string: &str, func: fn() -> T) -> T { + let result = { + let _lock = LOCALE_OVERRIDE_MUTEX + .lock() + .expect("Failed to get mutex lock for locale override"); + + let saved = std::env::var(LOCALE_OVERRIDE_ENV_VAR).ok(); + std::env::set_var(LOCALE_OVERRIDE_ENV_VAR, locale_string); + + let result = std::panic::catch_unwind(func); + + if let Some(locale_str) = saved { + std::env::set_var(LOCALE_OVERRIDE_ENV_VAR, locale_str); + } else { + std::env::remove_var(LOCALE_OVERRIDE_ENV_VAR); + } + + result + }; + + match result { + Ok(result) => result, + Err(err) => std::panic::resume_unwind(err), + } +} diff --git a/crates/nu-test-support/src/macros.rs b/crates/nu-test-support/src/macros.rs index 1f1768ffc3..4851baf6a7 100644 --- a/crates/nu-test-support/src/macros.rs +++ b/crates/nu-test-support/src/macros.rs @@ -1,21 +1,120 @@ +/// Run a command in nu and get it's output +/// +/// The `nu!` macro accepts a number of options like the `cwd` in which the +/// command should be run. It is also possible to specify a different `locale` +/// to test locale dependent commands. +/// +/// Pass options as the first arguments in the form of `key_1: value_1, key_1: +/// value_2, ...`. The options are defined in the `NuOpts` struct inside the +/// `nu!` macro. +/// +/// The command can be formatted using `{}` just like `println!` or `format!`. +/// Pass the format arguments comma separated after the command itself. +/// +/// # Examples +/// +/// ```no_run +/// # // NOTE: The `nu!` macro needs the `nu` binary to exist. The test are +/// # // therefore only compiled but not run (thats what the `no_run` at +/// # // the beginning of this code block is for). +/// # +/// use nu_test_support::nu; +/// +/// let outcome = nu!( +/// "date now | date to-record | get year" +/// ); +/// +/// let dir = "/"; +/// let outcome = nu!( +/// "ls {} | get name", +/// dir, +/// ); +/// +/// let outcome = nu!( +/// cwd: "/", +/// "ls | get name", +/// ); +/// +/// let cell = "size"; +/// let outcome = nu!( +/// locale: "de_DE.UTF-8", +/// "ls | into int {}", +/// cell, +/// ); +/// +/// let decimals = 2; +/// let outcome = nu!( +/// locale: "de_DE.UTF-8", +/// "10 | into string --decimals {}", +/// decimals, +/// ); +/// ``` #[macro_export] macro_rules! nu { - (cwd: $cwd:expr, $path:expr, $($part:expr),*) => {{ - use $crate::fs::DisplayPath; + // In the `@options` phase, we restucture all the + // `$field_1: $value_1, $field_2: $value_2, ...` + // pairs to a structure like + // `@options[ $field_1 => $value_1 ; $field_2 => $value_2 ; ... ]`. + // We do this to later distinguish the options from the `$path` and `$part`s. + // (See + // https://users.rust-lang.org/t/i-dont-think-this-local-ambiguity-when-calling-macro-is-ambiguous/79401?u=x3ro + // ) + // + // If there is any special treatment needed for the `$value`, we can just + // match for the specific `field` name. + ( + @options [ $($options:tt)* ] + cwd: $value:expr, + $($rest:tt)* + ) => { + nu!(@options [ $($options)* cwd => $crate::fs::in_directory($value) ; ] $($rest)*) + }; + // For all other options, we call `.into()` on the `$value` and hope for the best. ;) + ( + @options [ $($options:tt)* ] + $field:ident : $value:expr, + $($rest:tt)* + ) => { + nu!(@options [ $($options)* $field => $value.into() ; ] $($rest)*) + }; - let path = format!($path, $( - $part.display_path() - ),*); - - nu!($cwd, &path) + // When the `$field: $value,` pairs are all parsed, the next tokens are the `$path` and any + // number of `$part`s, potentially followed by a trailing comma. + ( + @options [ $($options:tt)* ] + $path:expr + $(, $part:expr)* + $(,)* + ) => {{ + // Here we parse the options into a `NuOpts` struct + let opts = nu!(@nu_opts $($options)*); + // and format the `$path` using the `$part`s + let path = nu!(@format_path $path, $($part),*); + // Then finally we go to the `@main` phase, where the actual work is done. + nu!(@main opts, path) }}; - (cwd: $cwd:expr, $path:expr) => {{ - nu!($cwd, $path) + // Create the NuOpts struct from the `field => value ;` pairs + (@nu_opts $( $field:ident => $value:expr ; )*) => { + NuOpts{ + $( + $field: Some($value), + )* + ..Default::default() + } + }; + + // Helper to format `$path`. + (@format_path $path:expr $(,)?) => { + // When there are no `$part`s, do not format anything + $path + }; + (@format_path $path:expr, $($part:expr),* $(,)?) => {{ + format!($path, $( $part ),*) }}; - ($cwd:expr, $path:expr) => {{ - pub use itertools::Itertools; + // Do the actual work. + (@main $opts:expr, $path:expr) => {{ pub use std::error::Error; pub use std::io::prelude::*; pub use std::process::{Command, Stdio}; @@ -36,13 +135,6 @@ macro_rules! nu { output } - // let commands = &*format!( - // " - // {} - // exit", - // $crate::fs::DisplayPath::display_path(&$path) - // ); - let test_bins = $crate::fs::binaries(); let cwd = std::env::current_dir().expect("Could not get current working directory."); @@ -64,21 +156,25 @@ macro_rules! nu { Err(_) => panic!("Couldn't join paths for PATH var."), }; - let target_cwd = $crate::fs::in_directory(&$cwd); + let target_cwd = $opts.cwd.unwrap_or(".".to_string()); + let locale = $opts.locale.unwrap_or("en_US.UTF-8".to_string()); - let mut process = match Command::new($crate::fs::executable_path()) + let mut command = Command::new($crate::fs::executable_path()); + command .env("PWD", &target_cwd) + .env(nu_utils::locale::LOCALE_OVERRIDE_ENV_VAR, locale) .current_dir(target_cwd) .env(NATIVE_PATH_ENV_VAR, paths_joined) // .arg("--skip-plugins") // .arg("--no-history") // .arg("--config-file") // .arg($crate::fs::DisplayPath::display_path(&$crate::fs::fixtures().join("playground/config/default.toml"))) - .arg(format!("-c {}", escape_quote_string($crate::fs::DisplayPath::display_path(&path)))) + .arg(format!("-c {}", escape_quote_string(path))) .stdout(Stdio::piped()) // .stdin(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() + .stderr(Stdio::piped()); + + let mut process = match command.spawn() { Ok(child) => child, Err(why) => panic!("Can't run test {:?} {}", $crate::fs::executable_path(), why.to_string()), @@ -100,6 +196,17 @@ macro_rules! nu { $crate::Outcome::new(out,err.into_owned()) }}; + + // This is the entrypoint for this macro. + ($($token:tt)*) => {{ + #[derive(Default)] + struct NuOpts { + cwd: Option, + locale: Option, + } + + nu!(@options [ ] $($token)*) + }}; } #[macro_export] diff --git a/crates/nu-test-support/tests/get_system_locale.rs b/crates/nu-test-support/tests/get_system_locale.rs new file mode 100644 index 0000000000..862b944181 --- /dev/null +++ b/crates/nu-test-support/tests/get_system_locale.rs @@ -0,0 +1,19 @@ +use nu_test_support::locale_override::with_locale_override; +use nu_utils::get_system_locale; +use num_format::Grouping; + +#[test] +fn test_get_system_locale_en() { + let locale = with_locale_override("en_US.UTF-8", || get_system_locale()); + + assert_eq!(locale.name(), "en"); + assert_eq!(locale.grouping(), Grouping::Standard) +} + +#[test] +fn test_get_system_locale_de() { + let locale = with_locale_override("de_DE.UTF-8", || get_system_locale()); + + assert_eq!(locale.name(), "de"); + assert_eq!(locale.grouping(), Grouping::Standard) +} diff --git a/crates/nu-utils/Cargo.toml b/crates/nu-utils/Cargo.toml index d325446d9c..f469e6ffcb 100644 --- a/crates/nu-utils/Cargo.toml +++ b/crates/nu-utils/Cargo.toml @@ -13,6 +13,8 @@ path = "src/main.rs" [dependencies] lscolors = { version = "0.12.0", features = ["crossterm"]} +num-format = { version = "0.4.0" } +sys-locale = "0.2.1" [target.'cfg(windows)'.dependencies] crossterm_winapi = "0.9.0" diff --git a/crates/nu-utils/src/lib.rs b/crates/nu-utils/src/lib.rs index 9199d05877..0eca65061b 100644 --- a/crates/nu-utils/src/lib.rs +++ b/crates/nu-utils/src/lib.rs @@ -1,5 +1,7 @@ +pub mod locale; pub mod utils; +pub use locale::get_system_locale; pub use utils::{ enable_vt_processing, get_default_config, get_default_env, get_ls_colors, stderr_write_all_and_flush, stdout_write_all_and_flush, diff --git a/crates/nu-utils/src/locale.rs b/crates/nu-utils/src/locale.rs new file mode 100644 index 0000000000..bcdfe20fbc --- /dev/null +++ b/crates/nu-utils/src/locale.rs @@ -0,0 +1,39 @@ +use num_format::Locale; + +#[cfg(debug_assertions)] +pub const LOCALE_OVERRIDE_ENV_VAR: &str = "NU_TEST_LOCALE_OVERRIDE"; + +pub fn get_system_locale() -> Locale { + let locale_string = get_system_locale_string().unwrap_or_else(|| String::from("en-US")); + // Since get_locale() and Locale::from_name() don't always return the same items + // we need to try and parse it to match. For instance, a valid locale is de_DE + // however Locale::from_name() wants only de so we split and parse it out. + let locale_string = locale_string.replace('_', "-"); // en_AU -> en-AU + + match Locale::from_name(&locale_string) { + Ok(loc) => loc, + _ => { + let all = num_format::Locale::available_names(); + let locale_prefix = &locale_string.split('-').collect::>(); + if all.contains(&locale_prefix[0]) { + // eprintln!("Found alternate: {}", &locale_prefix[0]); + Locale::from_name(locale_prefix[0]).unwrap_or(Locale::en) + } else { + // eprintln!("Unable to find matching locale. Defaulting to en-US"); + Locale::en + } + } + } +} + +#[cfg(debug_assertions)] +fn get_system_locale_string() -> Option { + std::env::var(LOCALE_OVERRIDE_ENV_VAR) + .ok() + .or_else(sys_locale::get_locale) +} + +#[cfg(not(debug_assertions))] +fn get_system_locale_string() -> Option { + sys_locale::get_locale() +} diff --git a/tests/shell/environment/mod.rs b/tests/shell/environment/mod.rs index 062488c103..faf884f166 100644 --- a/tests/shell/environment/mod.rs +++ b/tests/shell/environment/mod.rs @@ -12,9 +12,9 @@ pub mod support { pub fn in_path(dirs: &Dirs, block: impl FnOnce() -> Outcome) -> Outcome { let for_env_manifest = dirs.test().to_string_lossy(); - nu!(cwd: dirs.root(), format!("autoenv trust \"{}\"", for_env_manifest)); + nu!(cwd: dirs.root(), "autoenv trust \"{}\"", for_env_manifest); let out = block(); - nu!(cwd: dirs.root(), format!("autoenv untrust \"{}\"", for_env_manifest)); + nu!(cwd: dirs.root(), "autoenv untrust \"{}\"", for_env_manifest); out }