diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 336f63b..8ca5af3 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -1,6 +1,7 @@ // Module declarations mod dependency; pub(crate) mod layer; +mod todo_format; mod common_test; mod folder_privacy; @@ -282,6 +283,7 @@ fn validate(configuration: &Configuration) -> Vec { [&CheckerType::Layer] .clone(), }), + Box::new(todo_format::Checker), ]; let mut validation_errors: Vec = validators diff --git a/src/packs/checker/todo_format.rs b/src/packs/checker/todo_format.rs new file mode 100644 index 0000000..9e60fda --- /dev/null +++ b/src/packs/checker/todo_format.rs @@ -0,0 +1,130 @@ +//! 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; + +use crate::packs::bin_locater; +use crate::packs::{Configuration, PackageTodo}; +use anyhow::{anyhow, Result}; +use rayon::prelude::*; + +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 validation_errors: Vec = configuration + .pack_set + .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"); + + // 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() + .map(|e| e.to_string()) + }) + .collect(); + + if validation_errors.is_empty() { + None + } else { + Some(validation_errors) + } + } +} + +/// 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<()> { + // Read the current file content + 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)?; + + // Re-serialize using the standard serialization logic to get the expected format + let expected_content = crate::packs::package_todo::serialize_package_todo( + pack_name, + &package_todo, + packs_first_mode, + ); + + // Compare the current content with the expected serialized format + if current_content != expected_content { + 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(), + command + )); + } + + Ok(()) +} diff --git a/src/packs/package_todo.rs b/src/packs/package_todo.rs index 5c0f480..56cac4d 100644 --- a/src/packs/package_todo.rs +++ b/src/packs/package_todo.rs @@ -171,8 +171,35 @@ pub fn write_violations_to_disk( debug!("Finished writing violations to disk"); } -fn serialize_package_todo( - responsible_pack_name: &String, +/// 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: &str, package_todo: &PackageTodo, packs_first_mode: bool, ) -> String { @@ -223,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 { 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/todo_format_validation_test.rs b/tests/todo_format_validation_test.rs new file mode 100644 index 0000000..4b4f7ca --- /dev/null +++ b/tests/todo_format_validation_test.rs @@ -0,0 +1,95 @@ +//! 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") + .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(()) +} + +/// Tests that validation passes for correctly formatted package_todo.yml files. +/// +/// This test uses an existing fixture that has a properly formatted package_todo.yml +/// file (with correct ordering, headers, and format). The validation should succeed. +#[test] +fn test_validate_correctly_formatted_package_todo() -> Result<(), Box> +{ + 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(()) +} + +/// 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(()) +}