From bdbcf829673c0a51805499832c20fab8a010733d Mon Sep 17 00:00:00 2001 From: Darren Schroeder <343840+fdncred@users.noreply.github.com> Date: Sat, 12 Oct 2024 21:43:24 -0500 Subject: [PATCH] Revert "Add the `history import` command" (#14077) --- crates/nu-cli/src/commands/default_context.rs | 1 - crates/nu-cli/src/commands/history/fields.rs | 10 - .../nu-cli/src/commands/history/history_.rs | 31 +- .../src/commands/history/history_import.rs | 332 ------------------ crates/nu-cli/src/commands/history/mod.rs | 3 - crates/nu-cli/src/commands/mod.rs | 2 +- .../nu-cli/tests/commands/history_import.rs | 186 ---------- crates/nu-cli/tests/commands/mod.rs | 1 - crates/nu-test-support/src/macros.rs | 16 +- 9 files changed, 18 insertions(+), 564 deletions(-) delete mode 100644 crates/nu-cli/src/commands/history/fields.rs delete mode 100644 crates/nu-cli/src/commands/history/history_import.rs delete mode 100644 crates/nu-cli/tests/commands/history_import.rs diff --git a/crates/nu-cli/src/commands/default_context.rs b/crates/nu-cli/src/commands/default_context.rs index ad19f18d94..0c1c459c20 100644 --- a/crates/nu-cli/src/commands/default_context.rs +++ b/crates/nu-cli/src/commands/default_context.rs @@ -17,7 +17,6 @@ pub fn add_cli_context(mut engine_state: EngineState) -> EngineState { CommandlineGetCursor, CommandlineSetCursor, History, - HistoryImport, HistorySession, Keybindings, KeybindingsDefault, diff --git a/crates/nu-cli/src/commands/history/fields.rs b/crates/nu-cli/src/commands/history/fields.rs deleted file mode 100644 index f14a3e81ca..0000000000 --- a/crates/nu-cli/src/commands/history/fields.rs +++ /dev/null @@ -1,10 +0,0 @@ -// Each const is named after a HistoryItem field, and the value is the field name to be displayed to -// the user (or accept during import). -pub const COMMAND_LINE: &str = "command"; -pub const ID: &str = "item_id"; -pub const START_TIMESTAMP: &str = "start_timestamp"; -pub const HOSTNAME: &str = "hostname"; -pub const CWD: &str = "cwd"; -pub const EXIT_STATUS: &str = "exit_status"; -pub const DURATION: &str = "duration"; -pub const SESSION_ID: &str = "session_id"; diff --git a/crates/nu-cli/src/commands/history/history_.rs b/crates/nu-cli/src/commands/history/history_.rs index c31317882a..602574229e 100644 --- a/crates/nu-cli/src/commands/history/history_.rs +++ b/crates/nu-cli/src/commands/history/history_.rs @@ -5,8 +5,6 @@ use reedline::{ SqliteBackedHistory, }; -use super::fields; - #[derive(Clone)] pub struct History; @@ -85,8 +83,7 @@ impl Command for History { entries.into_iter().enumerate().map(move |(idx, entry)| { Value::record( record! { - fields::COMMAND_LINE => Value::string(entry.command_line, head), - // TODO: This name is inconsistent with create_history_record. + "command" => Value::string(entry.command_line, head), "index" => Value::int(idx as i64, head), }, head, @@ -178,14 +175,14 @@ fn create_history_record(idx: usize, entry: HistoryItem, long: bool, head: Span) if long { Value::record( record! { - fields::ID => item_id_value, - fields::START_TIMESTAMP => start_timestamp_value, - fields::COMMAND_LINE => command_value, - fields::SESSION_ID => session_id_value, - fields::HOSTNAME => hostname_value, - fields::CWD => cwd_value, - fields::DURATION => duration_value, - fields::EXIT_STATUS => exit_status_value, + "item_id" => item_id_value, + "start_timestamp" => start_timestamp_value, + "command" => command_value, + "session_id" => session_id_value, + "hostname" => hostname_value, + "cwd" => cwd_value, + "duration" => duration_value, + "exit_status" => exit_status_value, "idx" => index_value, }, head, @@ -193,11 +190,11 @@ fn create_history_record(idx: usize, entry: HistoryItem, long: bool, head: Span) } else { Value::record( record! { - fields::START_TIMESTAMP => start_timestamp_value, - fields::COMMAND_LINE => command_value, - fields::CWD => cwd_value, - fields::DURATION => duration_value, - fields::EXIT_STATUS => exit_status_value, + "start_timestamp" => start_timestamp_value, + "command" => command_value, + "cwd" => cwd_value, + "duration" => duration_value, + "exit_status" => exit_status_value, }, head, ) diff --git a/crates/nu-cli/src/commands/history/history_import.rs b/crates/nu-cli/src/commands/history/history_import.rs deleted file mode 100644 index 71daeac58c..0000000000 --- a/crates/nu-cli/src/commands/history/history_import.rs +++ /dev/null @@ -1,332 +0,0 @@ -use nu_engine::command_prelude::*; -use nu_protocol::HistoryFileFormat; - -use reedline::{ - FileBackedHistory, History, HistoryItem, HistoryItemId, ReedlineError, SearchQuery, - SqliteBackedHistory, -}; - -use super::fields; - -#[derive(Clone)] -pub struct HistoryImport; - -impl Command for HistoryImport { - fn name(&self) -> &str { - "history import" - } - - fn description(&self) -> &str { - "Import command line history" - } - - fn extra_description(&self) -> &str { - r#"Can import history from input, either successive command lines or more detailed records. If providing records, available fields are: - command_line, id, start_timestamp, hostname, cwd, duration, exit_status. - -If no input is provided, will import all history items from existing history in the other format: if current history is stored in sqlite, it will store it in plain text and vice versa."# - } - - fn signature(&self) -> nu_protocol::Signature { - Signature::build("history import") - .category(Category::History) - .input_output_types(vec![ - (Type::Nothing, Type::Nothing), - (Type::List(Box::new(Type::String)), Type::Nothing), - (Type::table(), Type::Nothing), - ]) - } - - fn examples(&self) -> Vec { - vec![ - Example { - example: "history import", - description: - "Append all items from history in the other format to the current history", - result: None, - }, - Example { - example: "echo foo | history import", - description: "Append `foo` to the current history", - result: None, - }, - Example { - example: "[[ command_line cwd ]; [ foo /home ]] | history import", - description: "Append `foo` ran from `/home` to the current history", - result: None, - }, - ] - } - - fn run( - &self, - engine_state: &EngineState, - _stack: &mut Stack, - call: &Call, - input: PipelineData, - ) -> Result { - let ok = Ok(Value::nothing(call.head).into_pipeline_data()); - - let Some(history) = engine_state.history_config() else { - return ok; - }; - let Some(current_history_path) = history.file_path() else { - return Err(ShellError::ConfigDirNotFound { - span: Some(call.head), - }); - }; - - match input { - PipelineData::Empty => { - let other_format = match history.file_format { - HistoryFileFormat::Sqlite => HistoryFileFormat::Plaintext, - HistoryFileFormat::Plaintext => HistoryFileFormat::Sqlite, - }; - let src = new_backend(other_format, None)?; - let mut dst = new_backend(history.file_format, Some(current_history_path))?; - let items = src - .search(SearchQuery::everything( - reedline::SearchDirection::Forward, - None, - )) - .map_err(error_from_reedline)? - .into_iter() - .map(Ok); - import(dst.as_mut(), items) - } - _ => { - let input = input.into_iter().map(item_from_value); - import( - new_backend(history.file_format, Some(current_history_path))?.as_mut(), - input, - ) - } - }?; - - ok - } -} - -fn new_backend( - format: HistoryFileFormat, - path: Option, -) -> Result, ShellError> { - let path = match path { - Some(path) => path, - None => { - let Some(mut path) = nu_path::nu_config_dir() else { - return Err(ShellError::ConfigDirNotFound { span: None }); - }; - path.push(format.default_file_name()); - path.into_std_path_buf() - } - }; - - fn map( - result: Result, - ) -> Result, ShellError> { - result - .map(|x| Box::new(x) as Box) - .map_err(error_from_reedline) - } - match format { - // Use a reasonably large value for maximum capacity. - HistoryFileFormat::Plaintext => map(FileBackedHistory::with_file(0xfffffff, path)), - HistoryFileFormat::Sqlite => map(SqliteBackedHistory::with_file(path, None, None)), - } -} - -fn import( - dst: &mut dyn History, - src: impl Iterator>, -) -> Result<(), ShellError> { - for item in src { - dst.save(item?).map_err(error_from_reedline)?; - } - Ok(()) -} - -fn error_from_reedline(e: ReedlineError) -> ShellError { - // TODO: Should we add a new ShellError variant? - ShellError::GenericError { - error: "Reedline error".to_owned(), - msg: format!("{e}"), - span: None, - help: None, - inner: Vec::new(), - } -} - -fn item_from_value(v: Value) -> Result { - let span = v.span(); - match v { - Value::Record { val, .. } => item_from_record(val.into_owned(), span), - Value::String { val, .. } => Ok(HistoryItem { - command_line: val, - id: None, - start_timestamp: None, - session_id: None, - hostname: None, - cwd: None, - duration: None, - exit_status: None, - more_info: None, - }), - _ => Err(ShellError::UnsupportedInput { - msg: "Only list and record inputs are supported".to_owned(), - input: v.get_type().to_string(), - msg_span: span, - input_span: span, - }), - } -} - -fn item_from_record(mut rec: Record, span: Span) -> Result { - let cmd = match rec.remove(fields::COMMAND_LINE) { - Some(v) => v.as_str()?.to_owned(), - None => { - return Err(ShellError::TypeMismatch { - err_message: format!("missing column: {}", fields::COMMAND_LINE), - span, - }) - } - }; - - fn get( - rec: &mut Record, - field: &'static str, - f: impl FnOnce(Value) -> Result, - ) -> Result, ShellError> { - rec.remove(field).map(f).transpose() - } - - let rec = &mut rec; - let item = HistoryItem { - command_line: cmd, - id: get(rec, fields::ID, |v| Ok(HistoryItemId::new(v.as_int()?)))?, - start_timestamp: get(rec, fields::START_TIMESTAMP, |v| Ok(v.as_date()?.to_utc()))?, - hostname: get(rec, fields::HOSTNAME, |v| Ok(v.as_str()?.to_owned()))?, - cwd: get(rec, fields::CWD, |v| Ok(v.as_str()?.to_owned()))?, - exit_status: get(rec, fields::EXIT_STATUS, |v| v.as_i64())?, - duration: get(rec, fields::DURATION, duration_from_value)?, - more_info: None, - // TODO: Currently reedline doesn't let you create session IDs. - session_id: None, - }; - - if !rec.is_empty() { - let cols = rec.columns().map(|s| s.as_str()).collect::>(); - return Err(ShellError::TypeMismatch { - err_message: format!("unsupported column names: {}", cols.join(", ")), - span, - }); - } - Ok(item) -} - -fn duration_from_value(v: Value) -> Result { - chrono::Duration::nanoseconds(v.as_duration()?) - .to_std() - .map_err(|_| ShellError::IOError { - msg: "negative duration not supported".to_string(), - }) -} - -#[cfg(test)] -mod tests { - use chrono::DateTime; - - use super::*; - - #[test] - fn test_item_from_value_string() -> Result<(), ShellError> { - let item = item_from_value(Value::string("foo", Span::unknown()))?; - assert_eq!( - item, - HistoryItem { - command_line: "foo".to_string(), - id: None, - start_timestamp: None, - session_id: None, - hostname: None, - cwd: None, - duration: None, - exit_status: None, - more_info: None - } - ); - Ok(()) - } - - #[test] - fn test_item_from_value_record() { - let span = Span::unknown(); - let rec = new_record(&[ - ("command", Value::string("foo", span)), - ("item_id", Value::int(1, span)), - ( - "start_timestamp", - Value::date( - DateTime::parse_from_rfc3339("1996-12-19T16:39:57-08:00").unwrap(), - span, - ), - ), - ("hostname", Value::string("localhost", span)), - ("cwd", Value::string("/home/test", span)), - ("duration", Value::duration(100_000_000, span)), - ("exit_status", Value::int(42, span)), - ]); - let item = item_from_value(rec).unwrap(); - assert_eq!( - item, - HistoryItem { - command_line: "foo".to_string(), - id: Some(HistoryItemId::new(1)), - start_timestamp: Some( - DateTime::parse_from_rfc3339("1996-12-19T16:39:57-08:00") - .unwrap() - .to_utc() - ), - hostname: Some("localhost".to_string()), - cwd: Some("/home/test".to_string()), - duration: Some(std::time::Duration::from_nanos(100_000_000)), - exit_status: Some(42), - - session_id: None, - more_info: None - } - ); - } - - #[test] - fn test_item_from_value_record_extra_field() { - let span = Span::unknown(); - let rec = new_record(&[ - ("command_line", Value::string("foo", span)), - ("id_nonexistent", Value::int(1, span)), - ]); - assert!(item_from_value(rec).is_err()); - } - - #[test] - fn test_item_from_value_record_bad_type() { - let span = Span::unknown(); - let rec = new_record(&[ - ("command_line", Value::string("foo", span)), - ("id", Value::string("one".to_string(), span)), - ]); - assert!(item_from_value(rec).is_err()); - } - - fn new_record(rec: &[(&'static str, Value)]) -> Value { - let span = Span::unknown(); - let rec = Record::from_raw_cols_vals( - rec.iter().map(|(col, _)| col.to_string()).collect(), - rec.iter().map(|(_, val)| val.clone()).collect(), - span, - span, - ) - .unwrap(); - Value::record(rec, span) - } -} diff --git a/crates/nu-cli/src/commands/history/mod.rs b/crates/nu-cli/src/commands/history/mod.rs index c36b560307..be7d1fc11f 100644 --- a/crates/nu-cli/src/commands/history/mod.rs +++ b/crates/nu-cli/src/commands/history/mod.rs @@ -1,8 +1,5 @@ -mod fields; mod history_; -mod history_import; mod history_session; pub use history_::History; -pub use history_import::HistoryImport; pub use history_session::HistorySession; diff --git a/crates/nu-cli/src/commands/mod.rs b/crates/nu-cli/src/commands/mod.rs index 4a9dd9ef21..f63724e95f 100644 --- a/crates/nu-cli/src/commands/mod.rs +++ b/crates/nu-cli/src/commands/mod.rs @@ -7,7 +7,7 @@ mod keybindings_list; mod keybindings_listen; pub use commandline::{Commandline, CommandlineEdit, CommandlineGetCursor, CommandlineSetCursor}; -pub use history::{History, HistoryImport, HistorySession}; +pub use history::{History, HistorySession}; pub use keybindings::Keybindings; pub use keybindings_default::KeybindingsDefault; pub use keybindings_list::KeybindingsList; diff --git a/crates/nu-cli/tests/commands/history_import.rs b/crates/nu-cli/tests/commands/history_import.rs deleted file mode 100644 index c92260f457..0000000000 --- a/crates/nu-cli/tests/commands/history_import.rs +++ /dev/null @@ -1,186 +0,0 @@ -use nu_test_support::{nu, Outcome}; -use reedline::{ - FileBackedHistory, History, HistoryItem, HistoryItemId, ReedlineError, SearchQuery, - SqliteBackedHistory, -}; -use tempfile::TempDir; - -struct Test { - cfg_dir: TempDir, -} - -impl Test { - fn new(history_format: &'static str) -> Self { - let cfg_dir = tempfile::Builder::new() - .prefix("history_import_test") - .tempdir() - .unwrap(); - // Assigning to $env.config.history.file_format seems to work only in startup - // configuration. - std::fs::write( - cfg_dir.path().join("env.nu"), - format!("$env.config.history.file_format = {history_format:?}"), - ) - .unwrap(); - Self { cfg_dir } - } - - fn nu(&self, cmd: &'static str) -> Outcome { - let env = [( - "XDG_CONFIG_HOME".to_string(), - self.cfg_dir.path().to_str().unwrap().to_string(), - )]; - let env_config = self.cfg_dir.path().join("env.nu"); - nu!(envs: env, env_config: env_config, cmd) - } - - fn open_plaintext(&self) -> Result { - FileBackedHistory::with_file(100, self.cfg_dir.path().join("nushell").join("history.txt")) - } - - fn open_sqlite(&self) -> Result { - SqliteBackedHistory::with_file( - self.cfg_dir.path().join("nushell").join("history.sqlite3"), - None, - None, - ) - } -} - -fn query_all(history: impl History) -> Result, ReedlineError> { - history.search(SearchQuery::everything( - reedline::SearchDirection::Forward, - None, - )) -} - -fn save_all(mut history: impl History, items: Vec) -> Result<(), ReedlineError> { - for item in items { - history.save(item)?; - } - Ok(()) -} - -const EMPTY_ITEM: HistoryItem = HistoryItem { - command_line: String::new(), - id: None, - start_timestamp: None, - session_id: None, - hostname: None, - cwd: None, - duration: None, - exit_status: None, - more_info: None, -}; - -#[test] -fn history_import_pipe_string() { - let test = Test::new("plaintext"); - let outcome = test.nu("echo bar | history import"); - - assert!(outcome.status.success()); - assert_eq!( - query_all(test.open_plaintext().unwrap()).unwrap(), - vec![HistoryItem { - id: Some(HistoryItemId::new(0)), - command_line: "bar".to_string(), - ..EMPTY_ITEM - }] - ); -} - -#[test] -fn history_import_pipe_record() { - let test = Test::new("sqlite"); - let outcome = test.nu("[[item_id command]; [42 some_command]] | history import"); - - assert!(outcome.status.success()); - assert_eq!( - query_all(test.open_sqlite().unwrap()).unwrap(), - vec![HistoryItem { - id: Some(HistoryItemId::new(42)), - command_line: "some_command".to_string(), - ..EMPTY_ITEM - }] - ); -} - -#[test] -fn history_import_plain_to_sqlite() { - let test = Test::new("sqlite"); - save_all( - test.open_plaintext().unwrap(), - vec![ - HistoryItem { - id: Some(HistoryItemId::new(0)), - command_line: "foo".to_string(), - ..EMPTY_ITEM - }, - HistoryItem { - id: Some(HistoryItemId::new(1)), - command_line: "bar".to_string(), - ..EMPTY_ITEM - }, - ], - ) - .unwrap(); - - let outcome = test.nu("history import"); - assert!(outcome.status.success()); - assert_eq!( - query_all(test.open_sqlite().unwrap()).unwrap(), - vec![ - HistoryItem { - id: Some(HistoryItemId::new(0)), - command_line: "foo".to_string(), - ..EMPTY_ITEM - }, - HistoryItem { - id: Some(HistoryItemId::new(1)), - command_line: "bar".to_string(), - ..EMPTY_ITEM - } - ] - ); -} - -#[test] -fn history_import_sqlite_to_plain() { - let test = Test::new("plaintext"); - save_all( - test.open_sqlite().unwrap(), - vec![ - HistoryItem { - id: Some(HistoryItemId::new(0)), - command_line: "foo".to_string(), - hostname: Some("host".to_string()), - ..EMPTY_ITEM - }, - HistoryItem { - id: Some(HistoryItemId::new(1)), - command_line: "bar".to_string(), - cwd: Some("/home/test".to_string()), - ..EMPTY_ITEM - }, - ], - ) - .unwrap(); - - let outcome = test.nu("history import"); - assert!(outcome.status.success()); - assert_eq!( - query_all(test.open_plaintext().unwrap()).unwrap(), - vec![ - HistoryItem { - id: Some(HistoryItemId::new(0)), - command_line: "foo".to_string(), - ..EMPTY_ITEM - }, - HistoryItem { - id: Some(HistoryItemId::new(1)), - command_line: "bar".to_string(), - ..EMPTY_ITEM - }, - ] - ); -} diff --git a/crates/nu-cli/tests/commands/mod.rs b/crates/nu-cli/tests/commands/mod.rs index 9eb18e3280..087791302e 100644 --- a/crates/nu-cli/tests/commands/mod.rs +++ b/crates/nu-cli/tests/commands/mod.rs @@ -1,3 +1,2 @@ -mod history_import; mod keybindings_list; mod nu_highlight; diff --git a/crates/nu-test-support/src/macros.rs b/crates/nu-test-support/src/macros.rs index 1ae7ce593e..2547b1f4c8 100644 --- a/crates/nu-test-support/src/macros.rs +++ b/crates/nu-test-support/src/macros.rs @@ -234,7 +234,7 @@ macro_rules! nu_with_plugins { } use crate::{Outcome, NATIVE_PATH_ENV_VAR}; -use nu_path::{AbsolutePath, AbsolutePathBuf, Path, PathBuf}; +use nu_path::{AbsolutePath, AbsolutePathBuf, Path}; use std::{ ffi::OsStr, process::{Command, Stdio}, @@ -248,10 +248,6 @@ pub struct NuOpts { pub envs: Option>, pub collapse_output: Option, pub use_ir: Option, - // Note: At the time this was added, passing in a file path was more convenient. However, - // passing in file contents seems like a better API - consider this when adding new uses of - // this field. - pub env_config: Option, } pub fn nu_run_test(opts: NuOpts, commands: impl AsRef, with_std: bool) -> Outcome { @@ -282,14 +278,8 @@ pub fn nu_run_test(opts: NuOpts, commands: impl AsRef, with_std: bool) -> O command.envs(envs); } - match opts.env_config { - Some(path) => command.arg("--env-config").arg(path), - // TODO: This seems unnecessary: the code that runs for integration tests - // (run_commands) loads startup configs only if they are specified via flags explicitly or - // the shell is started as logging shell (which it is not in this case). - None => command.arg("--no-config-file"), - }; - + // Ensure that the user's config doesn't interfere with the tests + command.arg("--no-config-file"); if !with_std { command.arg("--no-std-lib"); }