From 23a6fe39f4c2900163b16bd51805ef68a6d2d4d7 Mon Sep 17 00:00:00 2001 From: Ivy Evans Date: Wed, 27 Aug 2025 12:48:43 -0700 Subject: [PATCH 1/8] Add package_todo.yml format validation to validate command Adds validation to ensure package_todo.yml files maintain the correct serialization format, preventing manual edits from breaking the standard format and causing noise when running `pks update`. - Add PackageTodoFormatChecker validator that compares existing files with their expected serialized format - Integrate validator into existing validation system - Add comprehensive tests with fixtures for both valid and invalid formats - Show context-aware error messages suggesting correct update command based on packs_first_mode This prevents issues where mass search-replace operations (like renaming packs) result in incorrectly formatted files that create large changesets when people run `pks update`. --- src/packs/checker.rs | 6 +- src/packs/checker/package_todo.rs | 60 +++++++++++++++++++ src/packs/package_todo.rs | 2 +- .../packs/foo/package_todo.yml | 1 + .../package.yml | 2 + .../packs/foo/package.yml | 2 + .../packs/foo/package_todo.yml | 14 +++++ .../packwerk.yml | 6 ++ tests/package_todo_format_test.rs | 39 ++++++++++++ 9 files changed, 129 insertions(+), 3 deletions(-) create mode 100644 src/packs/checker/package_todo.rs create mode 100644 tests/fixtures/incorrectly_formatted_package_todo/package.yml create mode 100644 tests/fixtures/incorrectly_formatted_package_todo/packs/foo/package.yml create mode 100644 tests/fixtures/incorrectly_formatted_package_todo/packs/foo/package_todo.yml create mode 100644 tests/fixtures/incorrectly_formatted_package_todo/packwerk.yml create mode 100644 tests/package_todo_format_test.rs diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 336f63b..54a6436 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -1,6 +1,7 @@ // Module declarations mod dependency; pub(crate) mod layer; +mod package_todo; mod common_test; mod folder_privacy; @@ -14,7 +15,7 @@ use crate::packs::checker_configuration::CheckerType; // Internal imports use crate::packs::pack::write_pack_to_disk; use crate::packs::pack::Pack; -use crate::packs::package_todo; +use crate::packs::package_todo as package_todo_module; use crate::packs::Configuration; use anyhow::bail; @@ -282,6 +283,7 @@ fn validate(configuration: &Configuration) -> Vec { [&CheckerType::Layer] .clone(), }), + Box::new(package_todo::Checker), ]; let mut validation_errors: Vec = validators @@ -346,7 +348,7 @@ pub(crate) fn update(configuration: &Configuration) -> anyhow::Result<()> { &strict_violations.len() ); } - package_todo::write_violations_to_disk(configuration, violations); + package_todo_module::write_violations_to_disk(configuration, violations); println!("Successfully updated package_todo.yml files!"); Ok(()) diff --git a/src/packs/checker/package_todo.rs b/src/packs/checker/package_todo.rs new file mode 100644 index 0000000..9e50c2f --- /dev/null +++ b/src/packs/checker/package_todo.rs @@ -0,0 +1,60 @@ +use std::fs; +use std::path::Path; + +use crate::packs::{Configuration, PackageTodo}; + +use super::ValidatorInterface; + +pub struct Checker; + +impl ValidatorInterface for Checker { + fn validate(&self, configuration: &Configuration) -> Option> { + let mut validation_errors = Vec::new(); + + for pack in &configuration.pack_set.packs { + let package_todo_path = pack.yml.parent().unwrap().join("package_todo.yml"); + + if !package_todo_path.exists() { + continue; + } + + if let Err(error) = validate_package_todo_format(&package_todo_path, &pack.name, configuration.packs_first_mode) { + validation_errors.push(error); + } + } + + if validation_errors.is_empty() { + None + } else { + Some(validation_errors) + } + } +} + +fn validate_package_todo_format( + package_todo_path: &Path, + pack_name: &str, + packs_first_mode: bool, +) -> Result<(), String> { + let current_content = fs::read_to_string(package_todo_path) + .map_err(|e| format!("Failed to read {}: {}", package_todo_path.display(), e))?; + + let package_todo: PackageTodo = serde_yaml::from_str(¤t_content) + .map_err(|e| format!("Failed to parse {}: {}", package_todo_path.display(), e))?; + + let expected_content = crate::packs::package_todo::serialize_package_todo( + &pack_name.to_string(), + &package_todo, + packs_first_mode, + ); + + if current_content != expected_content { + return Err(format!( + "Package todo file {} is not in the expected format. Please run `{}` to fix it.", + package_todo_path.display(), + if packs_first_mode { "pks update" } else { "bin/packwerk update-todo" } + )); + } + + Ok(()) +} \ No newline at end of file diff --git a/src/packs/package_todo.rs b/src/packs/package_todo.rs index 5c0f480..73f4e84 100644 --- a/src/packs/package_todo.rs +++ b/src/packs/package_todo.rs @@ -171,7 +171,7 @@ pub fn write_violations_to_disk( debug!("Finished writing violations to disk"); } -fn serialize_package_todo( +pub(crate) fn serialize_package_todo( responsible_pack_name: &String, package_todo: &PackageTodo, packs_first_mode: bool, diff --git a/tests/fixtures/contains_package_todo/packs/foo/package_todo.yml b/tests/fixtures/contains_package_todo/packs/foo/package_todo.yml index 73cdc52..45b0d6e 100644 --- a/tests/fixtures/contains_package_todo/packs/foo/package_todo.yml +++ b/tests/fixtures/contains_package_todo/packs/foo/package_todo.yml @@ -5,6 +5,7 @@ # You can regenerate this file using the following command: # # bin/packwerk update-todo +--- packs/bar: "::Bar": violations: diff --git a/tests/fixtures/incorrectly_formatted_package_todo/package.yml b/tests/fixtures/incorrectly_formatted_package_todo/package.yml new file mode 100644 index 0000000..7882524 --- /dev/null +++ b/tests/fixtures/incorrectly_formatted_package_todo/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: false +enforce_dependencies: false \ No newline at end of file diff --git a/tests/fixtures/incorrectly_formatted_package_todo/packs/foo/package.yml b/tests/fixtures/incorrectly_formatted_package_todo/packs/foo/package.yml new file mode 100644 index 0000000..46361b1 --- /dev/null +++ b/tests/fixtures/incorrectly_formatted_package_todo/packs/foo/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: true +enforce_dependencies: true \ No newline at end of file diff --git a/tests/fixtures/incorrectly_formatted_package_todo/packs/foo/package_todo.yml b/tests/fixtures/incorrectly_formatted_package_todo/packs/foo/package_todo.yml new file mode 100644 index 0000000..dcc1b68 --- /dev/null +++ b/tests/fixtures/incorrectly_formatted_package_todo/packs/foo/package_todo.yml @@ -0,0 +1,14 @@ +# This file is incorrectly formatted (not ordered properly) +--- +packs/bar: + "::Baz": + violations: + - privacy + - dependency + files: + - packs/foo/app/services/foo.rb + "::Bar": + violations: + - dependency + files: + - packs/foo/app/services/foo.rb \ No newline at end of file diff --git a/tests/fixtures/incorrectly_formatted_package_todo/packwerk.yml b/tests/fixtures/incorrectly_formatted_package_todo/packwerk.yml new file mode 100644 index 0000000..cb64e17 --- /dev/null +++ b/tests/fixtures/incorrectly_formatted_package_todo/packwerk.yml @@ -0,0 +1,6 @@ +include: + - "**/*.rb" + - "**/*.rake" +exclude: + - "{bin,node_modules,script,tmp,vendor}/**/*" +packs_first_mode: false \ No newline at end of file diff --git a/tests/package_todo_format_test.rs b/tests/package_todo_format_test.rs new file mode 100644 index 0000000..4e71dcb --- /dev/null +++ b/tests/package_todo_format_test.rs @@ -0,0 +1,39 @@ +use assert_cmd::prelude::*; +use predicates::prelude::*; +use std::{error::Error, process::Command}; + +mod common; + +#[test] +fn test_validate_incorrectly_formatted_package_todo() -> Result<(), Box> { + Command::cargo_bin("pks") + .unwrap() + .arg("--project-root") + .arg("tests/fixtures/incorrectly_formatted_package_todo") + .arg("validate") + .assert() + .failure() + .stdout(predicate::str::contains("1 validation error(s) detected:")) + .stdout(predicate::str::contains("is not in the expected format")) + .stdout(predicate::str::contains("bin/packwerk update-todo")); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_validate_correctly_formatted_package_todo() -> Result<(), Box> { + // This test uses an existing fixture that should have correctly formatted package_todo.yml + Command::cargo_bin("pks") + .unwrap() + .arg("--project-root") + .arg("tests/fixtures/contains_package_todo") + .arg("validate") + .assert() + .success() + .stdout(predicate::str::contains("Packwerk validate succeeded!")); + + common::teardown(); + Ok(()) +} + From bed9c136c1d88af778137daa024f141e78fe8f4e Mon Sep 17 00:00:00 2001 From: Ivy Evans Date: Wed, 27 Aug 2025 12:54:15 -0700 Subject: [PATCH 2/8] Add comprehensive documentation for package_todo format validation Add detailed doc comments explaining the package_todo.yml format validation functionality to help future maintainers understand: - How the validation works (deserialize -> re-serialize -> compare) - Why it's needed (prevent format inconsistencies from manual edits) - Common causes of validation failures - The role of serialize_package_todo in maintaining format consistency Documentation covers: - Module-level overview of the validation purpose - Function-level details for validation logic - Test documentation explaining test scenarios - Implementation details for the serialization format --- src/packs/checker/package_todo.rs | 53 +++++++++++++++++++++++++++++++ src/packs/package_todo.rs | 27 ++++++++++++++++ tests/package_todo_format_test.rs | 16 +++++++++- 3 files changed, 95 insertions(+), 1 deletion(-) diff --git a/src/packs/checker/package_todo.rs b/src/packs/checker/package_todo.rs index 9e50c2f..8da6584 100644 --- a/src/packs/checker/package_todo.rs +++ b/src/packs/checker/package_todo.rs @@ -1,3 +1,10 @@ +//! Package todo format validation checker. +//! +//! This module provides validation to ensure that package_todo.yml files maintain +//! the correct serialization format. This prevents issues where manual edits +//! (such as mass search-replace operations when renaming packs) result in +//! incorrectly formatted files that create noise when running `pks update`. + use std::fs; use std::path::Path; @@ -5,15 +12,35 @@ use crate::packs::{Configuration, PackageTodo}; use super::ValidatorInterface; +/// Validator that checks package_todo.yml files for correct serialization format. +/// +/// This validator ensures that existing package_todo.yml files match their expected +/// serialized format by: +/// 1. Reading the current file content +/// 2. Deserializing it to a PackageTodo struct +/// 3. Re-serializing it using the standard serialization logic +/// 4. Comparing the result with the original file content +/// +/// If differences are found, it reports validation errors with context-aware +/// suggestions for the correct command to run based on packs_first_mode. pub struct Checker; impl ValidatorInterface for Checker { + /// Validates that all existing package_todo.yml files are correctly formatted. + /// + /// Iterates through all packs and checks their package_todo.yml files (if they exist) + /// to ensure they match the expected serialization format. + /// + /// # Returns + /// - `None` if all files are correctly formatted + /// - `Some(Vec)` containing error messages for incorrectly formatted files fn validate(&self, configuration: &Configuration) -> Option> { let mut validation_errors = Vec::new(); for pack in &configuration.pack_set.packs { let package_todo_path = pack.yml.parent().unwrap().join("package_todo.yml"); + // Skip packs that don't have package_todo.yml files if !package_todo_path.exists() { continue; } @@ -31,23 +58,49 @@ impl ValidatorInterface for Checker { } } +/// Validates the format of a single package_todo.yml file. +/// +/// This function implements the core validation logic: +/// 1. Reads the current file content +/// 2. Deserializes it to ensure it's valid YAML and matches PackageTodo structure +/// 3. Re-serializes it using the standard serialization logic +/// 4. Compares the result with the original content +/// +/// # Arguments +/// * `package_todo_path` - Path to the package_todo.yml file to validate +/// * `pack_name` - Name of the pack (used for generating the correct header) +/// * `packs_first_mode` - Whether the project uses packs.yml (affects command suggestions) +/// +/// # Returns +/// * `Ok(())` if the file is correctly formatted +/// * `Err(String)` with a descriptive error message if validation fails +/// +/// # Common causes of validation failures +/// - Missing `---` separator after header comments +/// - Incorrect ordering of violations or files (should be alphabetically sorted) +/// - Manual edits that break the standard serialization format +/// - Wrong header comment (should match packs_first_mode setting) fn validate_package_todo_format( package_todo_path: &Path, pack_name: &str, packs_first_mode: bool, ) -> Result<(), String> { + // Read the current file content let current_content = fs::read_to_string(package_todo_path) .map_err(|e| format!("Failed to read {}: {}", package_todo_path.display(), e))?; + // Deserialize to ensure the file is valid and can be parsed let package_todo: PackageTodo = serde_yaml::from_str(¤t_content) .map_err(|e| format!("Failed to parse {}: {}", package_todo_path.display(), e))?; + // Re-serialize using the standard serialization logic to get the expected format let expected_content = crate::packs::package_todo::serialize_package_todo( &pack_name.to_string(), &package_todo, packs_first_mode, ); + // Compare the current content with the expected serialized format if current_content != expected_content { return Err(format!( "Package todo file {} is not in the expected format. Please run `{}` to fix it.", diff --git a/src/packs/package_todo.rs b/src/packs/package_todo.rs index 73f4e84..f82f1b2 100644 --- a/src/packs/package_todo.rs +++ b/src/packs/package_todo.rs @@ -171,6 +171,33 @@ pub fn write_violations_to_disk( debug!("Finished writing violations to disk"); } +/// Serializes a PackageTodo struct into the standard package_todo.yml format. +/// +/// This function generates the canonical serialized representation of a package_todo.yml file, +/// including the standard header comments and properly formatted YAML content. The output +/// format is deterministic and consistent, which is critical for format validation. +/// +/// # Arguments +/// * `responsible_pack_name` - The name of the pack that owns this package_todo.yml file +/// * `package_todo` - The PackageTodo struct containing violation data to serialize +/// * `packs_first_mode` - Whether the project uses packs.yml (affects header command) +/// +/// # Returns +/// A String containing the complete package_todo.yml file content, including: +/// - Standard header comments explaining the file's purpose +/// - Appropriate regeneration command based on packs_first_mode +/// - YAML separator (`---`) +/// - Sorted violation data in the standard format +/// +/// # Format Details +/// The serialized format ensures: +/// - Violations are sorted alphabetically by defining pack, then constant name +/// - Files within each violation are sorted alphabetically +/// - Constant names are properly quoted (using a hack to work around serde_yaml limitations) +/// - The header matches the project's configuration (packs_first_mode) +/// +/// This function is used both for writing new package_todo.yml files and for +/// format validation to ensure existing files match the expected format. pub(crate) fn serialize_package_todo( responsible_pack_name: &String, package_todo: &PackageTodo, diff --git a/tests/package_todo_format_test.rs b/tests/package_todo_format_test.rs index 4e71dcb..e9a0eba 100644 --- a/tests/package_todo_format_test.rs +++ b/tests/package_todo_format_test.rs @@ -1,9 +1,20 @@ +//! Tests for package_todo.yml format validation. +//! +//! These tests verify that the `pks validate` command correctly identifies +//! when package_todo.yml files are not in the expected serialization format +//! and provides appropriate error messages and suggestions. + use assert_cmd::prelude::*; use predicates::prelude::*; use std::{error::Error, process::Command}; mod common; +/// Tests that validation fails for incorrectly formatted package_todo.yml files. +/// +/// This test uses a fixture with a package_todo.yml file that has violations in +/// the wrong order (::Baz should come after ::Bar when sorted alphabetically). +/// The validation should fail and suggest running the appropriate update command. #[test] fn test_validate_incorrectly_formatted_package_todo() -> Result<(), Box> { Command::cargo_bin("pks") @@ -21,9 +32,12 @@ fn test_validate_incorrectly_formatted_package_todo() -> Result<(), Box Result<(), Box> { - // This test uses an existing fixture that should have correctly formatted package_todo.yml Command::cargo_bin("pks") .unwrap() .arg("--project-root") From d8bb42b1072ebc403e5c463ebd22773bc1994dc1 Mon Sep 17 00:00:00 2001 From: Ivy Evans Date: Wed, 27 Aug 2025 12:58:13 -0700 Subject: [PATCH 3/8] Optimize package_todo validator to use parallel processing Replace sequential iteration with par_iter() to validate package_todo.yml files in parallel, improving performance for large codebases with many packages. --- src/packs/checker/package_todo.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/packs/checker/package_todo.rs b/src/packs/checker/package_todo.rs index 8da6584..54c9465 100644 --- a/src/packs/checker/package_todo.rs +++ b/src/packs/checker/package_todo.rs @@ -9,6 +9,7 @@ use std::fs; use std::path::Path; use crate::packs::{Configuration, PackageTodo}; +use rayon::prelude::*; use super::ValidatorInterface; @@ -35,20 +36,19 @@ impl ValidatorInterface for Checker { /// - `None` if all files are correctly formatted /// - `Some(Vec)` containing error messages for incorrectly formatted files fn validate(&self, configuration: &Configuration) -> Option> { - let mut validation_errors = Vec::new(); + let validation_errors: Vec = configuration.pack_set.packs + .par_iter() + .filter_map(|pack| { + let package_todo_path = pack.yml.parent().unwrap().join("package_todo.yml"); + + // Skip packs that don't have package_todo.yml files + if !package_todo_path.exists() { + return None; + } - for pack in &configuration.pack_set.packs { - let package_todo_path = pack.yml.parent().unwrap().join("package_todo.yml"); - - // Skip packs that don't have package_todo.yml files - if !package_todo_path.exists() { - continue; - } - - if let Err(error) = validate_package_todo_format(&package_todo_path, &pack.name, configuration.packs_first_mode) { - validation_errors.push(error); - } - } + validate_package_todo_format(&package_todo_path, &pack.name, configuration.packs_first_mode).err() + }) + .collect(); if validation_errors.is_empty() { None From 64730b8ad3c1d8832bbb415ce0e681f7b306a209 Mon Sep 17 00:00:00 2001 From: Ivy Evans Date: Wed, 27 Aug 2025 13:21:42 -0700 Subject: [PATCH 4/8] Fix string allocation issues in package_todo format validation Replace &String parameters with &str to avoid unnecessary heap allocations and improve API design. Addresses feedback from code review. - Change serialize_package_todo parameter from &String to &str - Change header function parameter from &String to &str - Remove unnecessary to_string() call when passing pack_name --- src/packs/checker/package_todo.rs | 2 +- src/packs/package_todo.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/packs/checker/package_todo.rs b/src/packs/checker/package_todo.rs index 54c9465..c454a6a 100644 --- a/src/packs/checker/package_todo.rs +++ b/src/packs/checker/package_todo.rs @@ -95,7 +95,7 @@ fn validate_package_todo_format( // Re-serialize using the standard serialization logic to get the expected format let expected_content = crate::packs::package_todo::serialize_package_todo( - &pack_name.to_string(), + pack_name, &package_todo, packs_first_mode, ); diff --git a/src/packs/package_todo.rs b/src/packs/package_todo.rs index f82f1b2..56cac4d 100644 --- a/src/packs/package_todo.rs +++ b/src/packs/package_todo.rs @@ -199,7 +199,7 @@ pub fn write_violations_to_disk( /// This function is used both for writing new package_todo.yml files and for /// format validation to ensure existing files match the expected format. pub(crate) fn serialize_package_todo( - responsible_pack_name: &String, + responsible_pack_name: &str, package_todo: &PackageTodo, packs_first_mode: bool, ) -> String { @@ -250,7 +250,7 @@ fn delete_package_todo_from_disk(responsible_pack: &Pack) { } } -fn header(responsible_pack_name: &String, packs_first_mode: bool) -> String { +fn header(responsible_pack_name: &str, packs_first_mode: bool) -> String { let command = if packs_first_mode { "pks update" } else { From 03fa1942e0f957c67ef91f1c14b20427264bcd62 Mon Sep 17 00:00:00 2001 From: Ivy Evans Date: Wed, 27 Aug 2025 14:24:33 -0700 Subject: [PATCH 5/8] Apply cargo fmt formatting --- src/packs/checker/package_todo.rs | 28 ++++++++++++++++++++-------- tests/package_todo_format_test.rs | 7 ++++--- 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/packs/checker/package_todo.rs b/src/packs/checker/package_todo.rs index c454a6a..19ec689 100644 --- a/src/packs/checker/package_todo.rs +++ b/src/packs/checker/package_todo.rs @@ -36,17 +36,25 @@ impl ValidatorInterface for Checker { /// - `None` if all files are correctly formatted /// - `Some(Vec)` containing error messages for incorrectly formatted files fn validate(&self, configuration: &Configuration) -> Option> { - let validation_errors: Vec = configuration.pack_set.packs + let validation_errors: Vec = configuration + .pack_set + .packs .par_iter() .filter_map(|pack| { - let package_todo_path = pack.yml.parent().unwrap().join("package_todo.yml"); - + let package_todo_path = + pack.yml.parent().unwrap().join("package_todo.yml"); + // Skip packs that don't have package_todo.yml files if !package_todo_path.exists() { return None; } - validate_package_todo_format(&package_todo_path, &pack.name, configuration.packs_first_mode).err() + validate_package_todo_format( + &package_todo_path, + &pack.name, + configuration.packs_first_mode, + ) + .err() }) .collect(); @@ -86,12 +94,16 @@ fn validate_package_todo_format( packs_first_mode: bool, ) -> Result<(), String> { // Read the current file content - let current_content = fs::read_to_string(package_todo_path) - .map_err(|e| format!("Failed to read {}: {}", package_todo_path.display(), e))?; + let current_content = + fs::read_to_string(package_todo_path).map_err(|e| { + format!("Failed to read {}: {}", package_todo_path.display(), e) + })?; // Deserialize to ensure the file is valid and can be parsed let package_todo: PackageTodo = serde_yaml::from_str(¤t_content) - .map_err(|e| format!("Failed to parse {}: {}", package_todo_path.display(), e))?; + .map_err(|e| { + format!("Failed to parse {}: {}", package_todo_path.display(), e) + })?; // Re-serialize using the standard serialization logic to get the expected format let expected_content = crate::packs::package_todo::serialize_package_todo( @@ -110,4 +122,4 @@ fn validate_package_todo_format( } Ok(()) -} \ No newline at end of file +} diff --git a/tests/package_todo_format_test.rs b/tests/package_todo_format_test.rs index e9a0eba..d4bde81 100644 --- a/tests/package_todo_format_test.rs +++ b/tests/package_todo_format_test.rs @@ -16,7 +16,8 @@ mod common; /// the wrong order (::Baz should come after ::Bar when sorted alphabetically). /// The validation should fail and suggest running the appropriate update command. #[test] -fn test_validate_incorrectly_formatted_package_todo() -> Result<(), Box> { +fn test_validate_incorrectly_formatted_package_todo( +) -> Result<(), Box> { Command::cargo_bin("pks") .unwrap() .arg("--project-root") @@ -37,7 +38,8 @@ fn test_validate_incorrectly_formatted_package_todo() -> Result<(), Box Result<(), Box> { +fn test_validate_correctly_formatted_package_todo() -> Result<(), Box> +{ Command::cargo_bin("pks") .unwrap() .arg("--project-root") @@ -50,4 +52,3 @@ fn test_validate_correctly_formatted_package_todo() -> Result<(), Box common::teardown(); Ok(()) } - From 08291abc7beca4fb2afc8328d5c22cdb8aedcd99 Mon Sep 17 00:00:00 2001 From: Ivy Evans Date: Tue, 23 Sep 2025 10:40:07 -0700 Subject: [PATCH 6/8] refactor: address feedback from code review - Remove unnecessary 'as package_todo_module' alias in checker.rs - Rename package_todo.rs to todo_format.rs for clarity - Replace unwrap() with explicit expect() for better error handling Co-authored-by: Sweta Sanghavi Co-authored-by: Denis Kisselev Co-authored-by: Martin Emde --- src/packs/checker.rs | 8 ++++---- src/packs/checker/{package_todo.rs => todo_format.rs} | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) rename src/packs/checker/{package_todo.rs => todo_format.rs} (97%) diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 54a6436..8ca5af3 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -1,7 +1,7 @@ // Module declarations mod dependency; pub(crate) mod layer; -mod package_todo; +mod todo_format; mod common_test; mod folder_privacy; @@ -15,7 +15,7 @@ use crate::packs::checker_configuration::CheckerType; // Internal imports use crate::packs::pack::write_pack_to_disk; use crate::packs::pack::Pack; -use crate::packs::package_todo as package_todo_module; +use crate::packs::package_todo; use crate::packs::Configuration; use anyhow::bail; @@ -283,7 +283,7 @@ fn validate(configuration: &Configuration) -> Vec { [&CheckerType::Layer] .clone(), }), - Box::new(package_todo::Checker), + Box::new(todo_format::Checker), ]; let mut validation_errors: Vec = validators @@ -348,7 +348,7 @@ pub(crate) fn update(configuration: &Configuration) -> anyhow::Result<()> { &strict_violations.len() ); } - package_todo_module::write_violations_to_disk(configuration, violations); + package_todo::write_violations_to_disk(configuration, violations); println!("Successfully updated package_todo.yml files!"); Ok(()) diff --git a/src/packs/checker/package_todo.rs b/src/packs/checker/todo_format.rs similarity index 97% rename from src/packs/checker/package_todo.rs rename to src/packs/checker/todo_format.rs index 19ec689..5623463 100644 --- a/src/packs/checker/package_todo.rs +++ b/src/packs/checker/todo_format.rs @@ -42,7 +42,7 @@ impl ValidatorInterface for Checker { .par_iter() .filter_map(|pack| { let package_todo_path = - pack.yml.parent().unwrap().join("package_todo.yml"); + pack.yml.parent().expect("Pack yml file should have a parent directory").join("package_todo.yml"); // Skip packs that don't have package_todo.yml files if !package_todo_path.exists() { From 6dca901b809b09def615105ed5d2b1b025729439 Mon Sep 17 00:00:00 2001 From: Ivy Evans Date: Tue, 23 Sep 2025 11:09:22 -0700 Subject: [PATCH 7/8] fix: improve error handling consistency in package_todo format validation - Migrate from custom Result<(), String> to anyhow::Result<()> for consistency with codebase - Remove manual map_err calls and leverage anyhow's automatic error conversion - Use bin_locater::packs_bin_name() for command suggestions in packs_first_mode - Maintain backward compatibility with bin/packwerk update-todo for legacy mode Addresses PR feedback to align error handling patterns and centralize command generation. --- src/packs/checker/todo_format.rs | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/packs/checker/todo_format.rs b/src/packs/checker/todo_format.rs index 5623463..9e60fda 100644 --- a/src/packs/checker/todo_format.rs +++ b/src/packs/checker/todo_format.rs @@ -8,7 +8,9 @@ use std::fs; use std::path::Path; +use crate::packs::bin_locater; use crate::packs::{Configuration, PackageTodo}; +use anyhow::{anyhow, Result}; use rayon::prelude::*; use super::ValidatorInterface; @@ -41,8 +43,11 @@ impl ValidatorInterface for Checker { .packs .par_iter() .filter_map(|pack| { - let package_todo_path = - pack.yml.parent().expect("Pack yml file should have a parent directory").join("package_todo.yml"); + let package_todo_path = pack + .yml + .parent() + .expect("Pack yml file should have a parent directory") + .join("package_todo.yml"); // Skip packs that don't have package_todo.yml files if !package_todo_path.exists() { @@ -55,6 +60,7 @@ impl ValidatorInterface for Checker { configuration.packs_first_mode, ) .err() + .map(|e| e.to_string()) }) .collect(); @@ -92,18 +98,12 @@ fn validate_package_todo_format( package_todo_path: &Path, pack_name: &str, packs_first_mode: bool, -) -> Result<(), String> { +) -> Result<()> { // Read the current file content - let current_content = - fs::read_to_string(package_todo_path).map_err(|e| { - format!("Failed to read {}: {}", package_todo_path.display(), e) - })?; + let current_content = fs::read_to_string(package_todo_path)?; // Deserialize to ensure the file is valid and can be parsed - let package_todo: PackageTodo = serde_yaml::from_str(¤t_content) - .map_err(|e| { - format!("Failed to parse {}: {}", package_todo_path.display(), e) - })?; + let package_todo: PackageTodo = serde_yaml::from_str(¤t_content)?; // Re-serialize using the standard serialization logic to get the expected format let expected_content = crate::packs::package_todo::serialize_package_todo( @@ -114,10 +114,15 @@ fn validate_package_todo_format( // Compare the current content with the expected serialized format if current_content != expected_content { - return Err(format!( + let command = if packs_first_mode { + format!("{} update", bin_locater::packs_bin_name()) + } else { + "bin/packwerk update-todo".to_string() + }; + return Err(anyhow!( "Package todo file {} is not in the expected format. Please run `{}` to fix it.", package_todo_path.display(), - if packs_first_mode { "pks update" } else { "bin/packwerk update-todo" } + command )); } From 8139c5653d83c4cf20405e47f5a1cacd97fe1374 Mon Sep 17 00:00:00 2001 From: Ivy Evans Date: Tue, 23 Sep 2025 11:28:51 -0700 Subject: [PATCH 8/8] test: improve todo format validation test coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Rename test file: package_todo_format_test.rs → todo_format_validation_test.rs - Add test for violations existing but no package_todo.yml files - Add test for clean case with no violations and no todo files - Correct test #2 to use privacy_violation_overrides fixture with actual violations - Ensure validator correctly skips packs without todo files in all scenarios --- ...test.rs => todo_format_validation_test.rs} | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) rename tests/{package_todo_format_test.rs => todo_format_validation_test.rs} (57%) diff --git a/tests/package_todo_format_test.rs b/tests/todo_format_validation_test.rs similarity index 57% rename from tests/package_todo_format_test.rs rename to tests/todo_format_validation_test.rs index d4bde81..4b4f7ca 100644 --- a/tests/package_todo_format_test.rs +++ b/tests/todo_format_validation_test.rs @@ -52,3 +52,44 @@ fn test_validate_correctly_formatted_package_todo() -> Result<(), Box common::teardown(); Ok(()) } + +/// Tests that validation passes when violations exist but no package_todo.yml files. +/// +/// This test uses a fixture with actual privacy violations but no package_todo.yml files +/// and verifies that the todo format validator still succeeds (it only validates existing +/// todo files, not whether violations should have todo files). +#[test] +fn test_validate_passes_when_violations_exist_but_no_todo_file( +) -> Result<(), Box> { + Command::cargo_bin("pks") + .unwrap() + .arg("--project-root") + .arg("tests/fixtures/privacy_violation_overrides") + .arg("validate") + .assert() + .success() + .stdout(predicate::str::contains("Packwerk validate succeeded!")); + + common::teardown(); + Ok(()) +} + +/// Tests that validation succeeds when there are no violations and no package_todo.yml files. +/// +/// This test uses a clean fixture with no violations and no package_todo.yml files +/// to verify the validator succeeds in the simplest case. +#[test] +fn test_validate_succeeds_when_no_violations_and_no_todo_files( +) -> Result<(), Box> { + Command::cargo_bin("pks") + .unwrap() + .arg("--project-root") + .arg("tests/fixtures/simple_app") + .arg("validate") + .assert() + .success() + .stdout(predicate::str::contains("Packwerk validate succeeded!")); + + common::teardown(); + Ok(()) +}