From ae14196c1ec0cb2817c87daa5f740d1e050ca6fe Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Sun, 23 Nov 2025 14:32:41 +0000 Subject: [PATCH 1/3] feat: Apply redundant file cleanup to all output files This change ensures that the redundant file cleanup logic is applied to all output files, including summaries and global summaries, not just plan files. The `Writers` struct is now stored in a mutable variable to ensure its `Drop` implementation is called, which triggers the cleanup. The `write_plan`, `write_summary`, and `write_global_summary` functions have been updated to accept mutable references to the writers, preventing them from being consumed. This change also adds e2e and unit tests to verify the file cleanup behavior for all output types. --- src/app.rs | 12 ++-- src/output.rs | 12 ++-- src/pretty_print.rs | 67 ++++++++++++++++++++++ tests/cleanup_tests.rs | 123 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 200 insertions(+), 14 deletions(-) diff --git a/src/app.rs b/src/app.rs index 849794c..11e5425 100644 --- a/src/app.rs +++ b/src/app.rs @@ -28,15 +28,11 @@ pub fn main_loop(config: &Config) -> anyhow::Result<(Sender<()>, Option, } -pub fn write_plan(all_life_lapses: &[LifeLapse], mut plan_writers: Vec) -> Result<()> { - for plan_writer in &mut plan_writers { +pub fn write_plan(all_life_lapses: &[LifeLapse], plan_writers: &mut [Writer]) -> Result<()> { + for plan_writer in plan_writers { write_to(|| format_lifelapses(all_life_lapses), plan_writer)?; } Ok(()) @@ -31,10 +31,10 @@ pub fn write_plan(all_life_lapses: &[LifeLapse], mut plan_writers: Vec) pub fn write_summary( all_summaries: &[(Timestamp, Summary)], - mut summary_writers: Vec, + summary_writers: &mut [Writer], ) -> Result<()> { for (ts, summary) in all_summaries { - for summary_writer in &mut summary_writers { + for summary_writer in &mut *summary_writers { write_to( || format_category_summary(compute_context_summary(summary), ts.date()), summary_writer, @@ -46,11 +46,11 @@ pub fn write_summary( pub fn write_global_summary( all_summaries: &[(Timestamp, Summary)], - mut summary_writers: Vec, + summary_writers: &mut [Writer], ) -> Result<()> { let only_summaries: Vec = all_summaries.iter().map(|(_, s)| s.clone()).collect(); let summary: Summary = merge_all_summaries(&only_summaries); - for summary_writer in &mut summary_writers { + for summary_writer in summary_writers { let date = Local::now().date(); write_to( || { diff --git a/src/pretty_print.rs b/src/pretty_print.rs index ec29053..b285789 100644 --- a/src/pretty_print.rs +++ b/src/pretty_print.rs @@ -317,4 +317,71 @@ mod tests { "File with different prefix should be preserved" ); } + + #[test] + fn test_cleanup_redundant_files_multiple_redundant() { + let dir = tempdir().unwrap(); + let dir_path = dir.path(); + let prefix = "test_prefix"; + + // Create multiple existing files + let file_path1 = dir_path.join(format!("{}_old1.txt", prefix)); + let file_path2 = dir_path.join(format!("{}_old2.txt", prefix)); + let mut file1 = File::create(&file_path1).unwrap(); + let mut file2 = File::create(&file_path2).unwrap(); + writeln!(file1, "line 1").unwrap(); + writeln!(file2, "line 1\nline 2").unwrap(); + + // New content is a superset of both + let new_content = "line 1\nline 2\nline 3\n"; + + cleanup_redundant_files(dir_path, prefix, new_content).unwrap(); + + assert!(!file_path1.exists(), "Old file 1 should be deleted"); + assert!(!file_path2.exists(), "Old file 2 should be deleted"); + } + + #[test] + fn test_cleanup_redundant_files_not_a_prefix_on_line_boundary() { + let dir = tempdir().unwrap(); + let dir_path = dir.path(); + let prefix = "test_prefix"; + + // Create an existing file + let file_path = dir_path.join(format!("{}_old.txt", prefix)); + let mut file = File::create(&file_path).unwrap(); + write!(file, "line 1 partial").unwrap(); + + // New content starts with the old one, but not on a line boundary + let new_content = "line 1 partial extra"; + + cleanup_redundant_files(dir_path, prefix, new_content).unwrap(); + + assert!( + file_path.exists(), + "Old file should be preserved (not a prefix on line boundary)" + ); + } + + #[test] + fn test_cleanup_redundant_files_new_content_shorter() { + let dir = tempdir().unwrap(); + let dir_path = dir.path(); + let prefix = "test_prefix"; + + // Create an existing file + let file_path = dir_path.join(format!("{}_old.txt", prefix)); + let mut file = File::create(&file_path).unwrap(); + writeln!(file, "line 1\nline 2").unwrap(); + + // New content is shorter + let new_content = "line 1\n"; + + cleanup_redundant_files(dir_path, prefix, new_content).unwrap(); + + assert!( + file_path.exists(), + "Old file should be preserved (new content is shorter)" + ); + } } diff --git a/tests/cleanup_tests.rs b/tests/cleanup_tests.rs index 4f7cdf3..9bd2e52 100644 --- a/tests/cleanup_tests.rs +++ b/tests/cleanup_tests.rs @@ -157,3 +157,126 @@ fn cleanup_preserves_non_redundant_files() { "Should have two plan files (history preserved)" ); } + +#[test] +fn cleanup_removes_redundant_summary_files() { + let dir = tempdir().unwrap(); + let activities_path = dir.path().join("activities.txt"); + let config_path = dir.path().join("config.toml"); + let output_dir = dir.path().join("history"); + + fs::create_dir(&output_dir).unwrap(); + + // Initial activities + fs::write(&activities_path, "2025-01-01 10:00\n1 0 Task 1 @work\n").unwrap(); + + fs::write( + &config_path, + dedent(&format!( + r#" + activity_paths = ["{}"] + [quadrants] + Q1 = ["@work"] + "#, + activities_path.to_str().unwrap() + )), + ) + .unwrap(); + + let mut cmd = assert_cmd::Command::from_std(Command::new(env!("CARGO_BIN_EXE_tiro"))); + cmd.arg("--config") + .arg(config_path.to_str().unwrap()) + .arg("--summary") + .arg(output_dir.to_str().unwrap()) + .arg("--quiet"); + + cmd.assert().success(); + + // Find generated file recursively + // Note: The file structure is history/YYYY-wWW/YYYY-MM-DD/filename.txt + let find_files = || { + let mut files = vec![]; + for entry in walkdir::WalkDir::new(&output_dir) { + let entry = entry.unwrap(); + if entry.file_type().is_file() && entry.path().extension().is_some_and(|e| e == "txt") { + files.push(entry.path().to_owned()); + } + } + files + }; + + let files_1 = find_files(); + assert_eq!( + files_1.len(), + 2, + "Should have created one summary file and one global summary file" + ); + let summary_file_1 = files_1 + .iter() + .find(|f| f.file_name().unwrap().to_str().unwrap().starts_with("summary")) + .unwrap() + .clone(); + let global_summary_file_1 = files_1 + .iter() + .find(|f| { + f.file_name() + .unwrap() + .to_str() + .unwrap() + .starts_with("global_summary") + }) + .unwrap() + .clone(); + + // Run again with an additional activity with a new category. + // This should result in a summary that is a superset of the previous one. + fs::write( + &activities_path, + "2025-01-01 10:00\n1 0 Task 1 @work\n1 0 Task 2 @zen\n", + ) + .unwrap(); + + let mut cmd = assert_cmd::Command::from_std(Command::new(env!("CARGO_BIN_EXE_tiro"))); + cmd.arg("--config") + .arg(config_path.to_str().unwrap()) + .arg("--summary") + .arg(output_dir.to_str().unwrap()) + .arg("--quiet"); + + cmd.assert().success(); + + let files_2 = find_files(); + assert_eq!( + files_2.len(), + 2, + "Should still have two summary files (redundant one deleted)" + ); + + let summary_file_2 = files_2 + .iter() + .find(|f| f.file_name().unwrap().to_str().unwrap().starts_with("summary")) + .unwrap() + .clone(); + let global_summary_file_2 = files_2 + .iter() + .find(|f| { + f.file_name() + .unwrap() + .to_str() + .unwrap() + .starts_with("global_summary") + }) + .unwrap() + .clone(); + + assert!(!summary_file_1.exists(), "Old summary file should be deleted"); + assert!( + !global_summary_file_1.exists(), + "Old global summary file should be deleted" + ); + assert!(summary_file_2.exists(), "New summary file should exist"); + assert!( + global_summary_file_2.exists(), + "New global summary file should exist" + ); +} From 5c7fb521d7f27719459a10c55629a2de04f5ea55 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Sun, 23 Nov 2025 14:37:21 +0000 Subject: [PATCH 2/3] feat: Apply redundant file cleanup to all output files This change ensures that the redundant file cleanup logic is applied to all output files, including summaries and global summaries, not just plan files. The `Writers` struct is now stored in a mutable variable to ensure its `Drop` implementation is called, which triggers the cleanup. The `write_plan`, `write_summary`, and `write_global_summary` functions have been updated to accept mutable references to the writers, preventing them from being consumed. This change also adds e2e and unit tests to verify the file cleanup behavior for all output types. --- tests/cleanup_tests.rs | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/tests/cleanup_tests.rs b/tests/cleanup_tests.rs index 9bd2e52..a8c83bc 100644 --- a/tests/cleanup_tests.rs +++ b/tests/cleanup_tests.rs @@ -213,7 +213,13 @@ fn cleanup_removes_redundant_summary_files() { ); let summary_file_1 = files_1 .iter() - .find(|f| f.file_name().unwrap().to_str().unwrap().starts_with("summary")) + .find(|f| { + f.file_name() + .unwrap() + .to_str() + .unwrap() + .starts_with("summary") + }) .unwrap() .clone(); let global_summary_file_1 = files_1 @@ -254,7 +260,13 @@ fn cleanup_removes_redundant_summary_files() { let summary_file_2 = files_2 .iter() - .find(|f| f.file_name().unwrap().to_str().unwrap().starts_with("summary")) + .find(|f| { + f.file_name() + .unwrap() + .to_str() + .unwrap() + .starts_with("summary") + }) .unwrap() .clone(); let global_summary_file_2 = files_2 @@ -269,7 +281,10 @@ fn cleanup_removes_redundant_summary_files() { .unwrap() .clone(); - assert!(!summary_file_1.exists(), "Old summary file should be deleted"); + assert!( + !summary_file_1.exists(), + "Old summary file should be deleted" + ); assert!( !global_summary_file_1.exists(), "Old global summary file should be deleted" From 3298233e0de3839a53e8d2dcb0c922aeba3d954f Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Sun, 23 Nov 2025 15:06:42 +0000 Subject: [PATCH 3/3] feat: Apply redundant file cleanup to all output files This change ensures that the redundant file cleanup logic is applied to all output files, including summaries and global summaries, not just plan files. The `Writers` struct is now stored in a mutable variable to ensure its `Drop` implementation is called, which triggers the cleanup. The `write_plan`, `write_summary`, and `write_global_summary` functions have been updated to accept mutable references to the writers, preventing them from being consumed. This change also adds e2e and unit tests to verify the file cleanup behavior for all output types. The unit tests have been made more robust by adding assertions to verify the existence of files before the cleanup logic is called. --- src/pretty_print.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/pretty_print.rs b/src/pretty_print.rs index b285789..705eccd 100644 --- a/src/pretty_print.rs +++ b/src/pretty_print.rs @@ -245,6 +245,7 @@ mod tests { let mut file = File::create(&file_path).unwrap(); writeln!(file, "line 1").unwrap(); writeln!(file, "line 2").unwrap(); + assert!(file_path.exists(), "File should exist before cleanup"); // New content is a superset let new_content = "line 1\nline 2\nline 3\n"; @@ -265,6 +266,7 @@ mod tests { let mut file = File::create(&file_path).unwrap(); writeln!(file, "line 1").unwrap(); writeln!(file, "line 2").unwrap(); + assert!(file_path.exists(), "File should exist before cleanup"); // New content is different let new_content = "line 1\nline 3\n"; @@ -284,6 +286,7 @@ mod tests { let file_path = dir_path.join(format!("{}_old.txt", prefix)); let mut file = File::create(&file_path).unwrap(); writeln!(file, "line 1").unwrap(); + assert!(file_path.exists(), "File should exist before cleanup"); // New content is exact match let new_content = "line 1\n"; @@ -306,6 +309,7 @@ mod tests { let file_path = dir_path.join("other_prefix_old.txt"); let mut file = File::create(&file_path).unwrap(); writeln!(file, "line 1").unwrap(); + assert!(file_path.exists(), "File should exist before cleanup"); // New content is superset let new_content = "line 1\nline 2\n"; @@ -331,6 +335,8 @@ mod tests { let mut file2 = File::create(&file_path2).unwrap(); writeln!(file1, "line 1").unwrap(); writeln!(file2, "line 1\nline 2").unwrap(); + assert!(file_path1.exists(), "File 1 should exist before cleanup"); + assert!(file_path2.exists(), "File 2 should exist before cleanup"); // New content is a superset of both let new_content = "line 1\nline 2\nline 3\n"; @@ -351,6 +357,7 @@ mod tests { let file_path = dir_path.join(format!("{}_old.txt", prefix)); let mut file = File::create(&file_path).unwrap(); write!(file, "line 1 partial").unwrap(); + assert!(file_path.exists(), "File should exist before cleanup"); // New content starts with the old one, but not on a line boundary let new_content = "line 1 partial extra"; @@ -373,6 +380,7 @@ mod tests { let file_path = dir_path.join(format!("{}_old.txt", prefix)); let mut file = File::create(&file_path).unwrap(); writeln!(file, "line 1\nline 2").unwrap(); + assert!(file_path.exists(), "File should exist before cleanup"); // New content is shorter let new_content = "line 1\n";