From 795230f36bb12f3bcd451f48f86d8e901d99ddcb Mon Sep 17 00:00:00 2001 From: Zhenping Zhao Date: Fri, 13 Dec 2024 13:39:31 -0800 Subject: [PATCH] Follow review comments, and adjust expectation on test results for windows. Hide env_vars for PWD-per-drive from being collected by auto completion. --- Cargo.lock | 1 + crates/nu-cli/Cargo.toml | 2 ++ .../src/completions/variable_completions.rs | 10 ++++++ crates/nu-cli/tests/completions/mod.rs | 10 ------ .../nu-protocol/src/engine/pwd_per_drive.rs | 31 +++++++++++-------- tests/path/canonicalize.rs | 21 +++++++++++++ tests/plugins/env.rs | 9 +++++- 7 files changed, 60 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b3a686b8cd..65886fe183 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3256,6 +3256,7 @@ dependencies = [ "nu-utils", "percent-encoding", "reedline", + "regex", "rstest", "sysinfo 0.32.0", "tempfile", diff --git a/crates/nu-cli/Cargo.toml b/crates/nu-cli/Cargo.toml index 993bb1ac72..2fa9161a0c 100644 --- a/crates/nu-cli/Cargo.toml +++ b/crates/nu-cli/Cargo.toml @@ -42,6 +42,8 @@ sysinfo = { workspace = true } unicode-segmentation = { workspace = true } uuid = { workspace = true, features = ["v4"] } which = { workspace = true } +#[cfg(windows)] +regex = { workspace = true } [features] plugin = ["nu-plugin-engine"] diff --git a/crates/nu-cli/src/completions/variable_completions.rs b/crates/nu-cli/src/completions/variable_completions.rs index 69e0985e80..a1a8d68bd5 100644 --- a/crates/nu-cli/src/completions/variable_completions.rs +++ b/crates/nu-cli/src/completions/variable_completions.rs @@ -46,7 +46,17 @@ impl Completer for VariableCompletion { if !var_str.is_empty() { // Completion for $env. if var_str == "$env" { + #[cfg(not(windows))] let env_vars = stack.get_env_vars(working_set.permanent_state); + #[cfg(windows)] + let mut env_vars = stack.get_env_vars(working_set.permanent_state); + #[cfg(windows)] + { + // Hide env_vars for PWD-per-drive from being listed in completion selection + if let Ok(pattern) = regex::Regex::new(r"^=[a-zA-Z]:$") { + env_vars.retain(|key, _| !pattern.is_match(key)); + } + } // Return nested values if sublevels_count > 0 { diff --git a/crates/nu-cli/tests/completions/mod.rs b/crates/nu-cli/tests/completions/mod.rs index cdd428dbc9..e32ac8c4f2 100644 --- a/crates/nu-cli/tests/completions/mod.rs +++ b/crates/nu-cli/tests/completions/mod.rs @@ -1414,10 +1414,7 @@ fn variables_completions() { // Test completions for $env let suggestions = completer.complete("$env.", 5); - #[cfg(not(windows))] assert_eq!(3, suggestions.len()); - #[cfg(windows)] - assert_eq!(4, suggestions.len()); #[cfg(windows)] let expected: Vec = vec!["Path".into(), "PWD".into(), "TEST".into()]; @@ -1425,14 +1422,7 @@ fn variables_completions() { let expected: Vec = vec!["PATH".into(), "PWD".into(), "TEST".into()]; // Match results - #[cfg(not(windows))] match_suggestions(&expected, &suggestions); - #[cfg(windows)] - { - let mut suggestions = suggestions; - let _ = suggestions.remove(0); - match_suggestions(&expected, &suggestions); - } // Test completions for $env let suggestions = completer.complete("$env.T", 6); diff --git a/crates/nu-protocol/src/engine/pwd_per_drive.rs b/crates/nu-protocol/src/engine/pwd_per_drive.rs index bc43707953..941576d658 100644 --- a/crates/nu-protocol/src/engine/pwd_per_drive.rs +++ b/crates/nu-protocol/src/engine/pwd_per_drive.rs @@ -1,4 +1,6 @@ use crate::engine::{EngineState, Stack}; +#[cfg(windows)] +use omnipath::sys_absolute; use std::path::{Path, PathBuf}; // For file system command usage @@ -40,28 +42,34 @@ pub mod windows { fn maintain(&mut self, key: String, value: Value); } - macro_rules! impl_env_maintainer { - ($type:ty) => { - impl EnvMaintainer for $type { - fn maintain(&mut self, key: String, value: Value) { - self.add_env_var(key, value); - } - } - }; + impl EnvMaintainer for EngineState { + fn maintain(&mut self, key: String, value: Value) { + self.add_env_var(key, value); + } } - impl_env_maintainer!(Stack); - impl_env_maintainer!(EngineState); + impl EnvMaintainer for Stack { + fn maintain(&mut self, key: String, value: Value) { + self.add_env_var(key, value); + } + } /// For maintainer to notify PWD /// When user changes current directory, maintainer calls set_pwd() /// and PWD-per-drive call maintainer back to store it for the /// env_var_for_drive. + /// TBD: If value can't be converted to String or the value is not valid for + /// windows path on a drive, should 'cd' or 'auto_cd' get warning message + /// that PWD-per-drive can't process the path value? pub fn set_pwd(maintainer: &mut T, value: Value) { if let Ok(path_string) = String::from_value(value.clone()) { if let Some(drive) = extract_drive_letter(&path_string) { maintainer.maintain(env_var_for_drive(drive), value); + } else { + log::trace!("PWD-per-drive can't find drive of {}", path_string); } + } else if let Err(e) = value.into_string() { + log::trace!("PWD-per-drive can't keep this path value: {}", e); } } @@ -154,9 +162,6 @@ pub mod windows { /// Call Windows system API (via omnipath crate) to expand /// absolute path fn get_full_path_name_w(path_str: &str) -> Option { - use omnipath::sys_absolute; - use std::path::Path; - if let Ok(path_sys_abs) = sys_absolute(Path::new(path_str)) { Some(path_sys_abs.to_str()?.to_string()) } else { diff --git a/tests/path/canonicalize.rs b/tests/path/canonicalize.rs index f714480779..f1de46eced 100644 --- a/tests/path/canonicalize.rs +++ b/tests/path/canonicalize.rs @@ -100,6 +100,13 @@ fn canonicalize_dot() { let actual = canonicalize_with(".", expected.as_path()).expect("Failed to canonicalize"); + // Once my windows gives the current directory for different case + // Actual "E:\\Study", got "E:\\study" + #[cfg(windows)] + let actual = std::path::PathBuf::from(actual.to_str().unwrap().to_ascii_uppercase()); + #[cfg(windows)] + let expected = std::path::PathBuf::from(expected.to_str().unwrap().to_ascii_uppercase()); + assert_eq!(actual, expected); } @@ -110,6 +117,13 @@ fn canonicalize_many_dots() { let actual = canonicalize_with("././/.//////./././//.///", expected.as_path()) .expect("Failed to canonicalize"); + // Once my windows gives the current directory for different case + // Actual "E:\\Study", got "E:\\study" + #[cfg(windows)] + let actual = std::path::PathBuf::from(actual.to_str().unwrap().to_ascii_uppercase()); + #[cfg(windows)] + let expected = std::path::PathBuf::from(expected.to_str().unwrap().to_ascii_uppercase()); + assert_eq!(actual, expected); } @@ -148,6 +162,13 @@ fn canonicalize_double_dot() { .parent() .expect("Could not get parent of current directory"); + // Once my windows gives the current directory for different case + // Actual "E:\\Study", got "E:\\study" + #[cfg(windows)] + let actual = std::path::PathBuf::from(actual.to_str().unwrap().to_ascii_uppercase()); + #[cfg(windows)] + let expected = std::path::PathBuf::from(expected.to_str().unwrap().to_ascii_uppercase()); + assert_eq!(actual, expected); } diff --git a/tests/plugins/env.rs b/tests/plugins/env.rs index d7d1555f02..8d6b95b726 100644 --- a/tests/plugins/env.rs +++ b/tests/plugins/env.rs @@ -49,7 +49,14 @@ fn get_current_dir() { cwd.chars().next().unwrap().to_ascii_uppercase(), result.out.chars().next().unwrap().to_ascii_uppercase() ); - assert_eq!(cwd[1..], result.out[1..]); + // Once my windows gives the current directory for different case + // Actual "E:\\Study", got "E:\\study" + //left: ":\\study\\nushell\\tests" + //right: ":\\Study\\nushell\\tests" + assert_eq!( + cwd[1..].to_ascii_uppercase(), + result.out[1..].to_ascii_uppercase() + ); } }