From 636af7023cc6c5a0fd92c98c953ed1696c31f4c5 Mon Sep 17 00:00:00 2001 From: Evan Doyle Date: Mon, 27 Jan 2025 11:37:01 -0800 Subject: [PATCH] Centralize Diagnostic formatting, expose from Rust --- Cargo.lock | 28 +++++- Cargo.toml | 1 + python/tach/cli.py | 21 ++-- python/tach/extension.pyi | 4 + python/tests/test_check.py | 2 +- python/tests/test_cli.py | 24 +---- src/cli.rs | 52 ++++++++++ src/commands/check/format.rs | 172 ++++++++++++++++++++++++++++++++ src/commands/check/formatter.rs | 3 - src/commands/check/mod.rs | 1 + src/diagnostics.rs | 20 +++- src/lib.rs | 9 ++ 12 files changed, 296 insertions(+), 41 deletions(-) create mode 100644 src/commands/check/format.rs delete mode 100644 src/commands/check/formatter.rs diff --git a/Cargo.lock b/Cargo.lock index 683dea0e..382b1424 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -125,6 +125,19 @@ version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "613afe47fcd5fac7ccf1db93babcb082c5994d996f20b8b159f2ad1658eb5724" +[[package]] +name = "console" +version = "0.15.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ea3c6ecd8059b57859df5c69830340ed3c41d30e3da0c1cbed90a96ac853041b" +dependencies = [ + "encode_unicode", + "libc", + "once_cell", + "unicode-width 0.2.0", + "windows-sys 0.59.0", +] + [[package]] name = "crc32fast" version = "1.4.2" @@ -240,6 +253,12 @@ version = "1.13.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "60b1af1c220855b6ceac025d3f6ecdd2b7c4894bfe9cd9bda4fbb4bc7c0d4cf0" +[[package]] +name = "encode_unicode" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "34aa73646ffb006b8f5147f3dc182bd4bcb190227ce861fc4a4844bf8e3cb2c0" + [[package]] name = "equivalent" version = "1.0.1" @@ -403,7 +422,7 @@ version = "0.2.21" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "14dbbfd5c71d70241ecf9e6f13737f7b5ce823821063188d7e46c41d371eebd5" dependencies = [ - "unicode-width", + "unicode-width 0.1.13", ] [[package]] @@ -1306,6 +1325,7 @@ name = "tach" version = "0.23.0" dependencies = [ "cached", + "console", "crossbeam-channel", "ctrlc", "glob", @@ -1463,6 +1483,12 @@ version = "0.1.14" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7dd6e30e90baa6f72411720665d41d89b9a3d039dc45b8faea1ddd07f617f6af" +[[package]] +name = "unicode-width" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1fc81956842c57dac11422a97c3b8195a1ff727f06e85c84ed2e8aa277c9a0fd" + [[package]] name = "unicode_names2" version = "1.3.0" diff --git a/Cargo.toml b/Cargo.toml index 3b23bd46..7503d7b3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,6 +33,7 @@ rayon = "1.10.0" parking_lot = "0.12.3" itertools = "0.14.0" toml_edit = "0.22.22" +console = "0.15.10" [features] extension-module = ["pyo3/extension-module"] diff --git a/python/tach/cli.py b/python/tach/cli.py index 1d37eaf2..3d21ff9f 100644 --- a/python/tach/cli.py +++ b/python/tach/cli.py @@ -27,6 +27,7 @@ check_computation_cache, create_computation_cache_key, detect_unused_dependencies, + format_diagnostics, run_server, serialize_diagnostics_json, update_computation_cache, @@ -644,7 +645,11 @@ def tach_check( json.dump({"error": str(e)}, sys.stdout) sys.exit(1 if has_errors else 0) - print_diagnostics(diagnostics, project_root=project_root) + if diagnostics: + print( + format_diagnostics(project_root=project_root, diagnostics=diagnostics), + file=sys.stderr, + ) exit_code = 1 if has_errors else 0 # If we're checking in exact mode, we want to verify that there are no unused dependencies @@ -696,14 +701,14 @@ def tach_check_external( exclude_paths=exclude_paths, ) - warnings = list(filter(lambda d: d.is_warning(), diagnostics)) - errors = list(filter(lambda d: d.is_error(), diagnostics)) - for diagnostic in warnings: - print(diagnostic.to_string()) - for diagnostic in errors: - print(diagnostic.to_string()) + if diagnostics: + print( + format_diagnostics(project_root=project_root, diagnostics=diagnostics), + file=sys.stderr, + ) - if errors: + has_errors = any(diagnostic.is_error() for diagnostic in diagnostics) + if has_errors: sys.exit(1) else: print( diff --git a/python/tach/extension.pyi b/python/tach/extension.pyi index 0072b73d..0c896c7b 100644 --- a/python/tach/extension.pyi +++ b/python/tach/extension.pyi @@ -61,6 +61,10 @@ def check_external_dependencies( module_mappings: dict[str, list[str]], stdlib_modules: list[str], ) -> list[Diagnostic]: ... +def format_diagnostics( + project_root: Path, + diagnostics: list[Diagnostic], +) -> str: ... def detect_unused_dependencies( project_root: Path, project_config: ProjectConfig, diff --git a/python/tests/test_check.py b/python/tests/test_check.py index 6443a726..60d325ac 100644 --- a/python/tests/test_check.py +++ b/python/tests/test_check.py @@ -25,7 +25,7 @@ def test_valid_example_dir(example_dir, capfd): assert exc_info.value.code == 0 captured = capfd.readouterr() assert SUCCESS in captured.out - assert WARNING in captured.err + assert WARNING in captured.err or "WARN" in captured.err def test_valid_example_dir_monorepo(example_dir): diff --git a/python/tests/test_cli.py b/python/tests/test_cli.py index fdc54da4..6ed04640 100644 --- a/python/tests/test_cli.py +++ b/python/tests/test_cli.py @@ -6,7 +6,7 @@ import pytest from tach import cli -from tach.extension import Diagnostic, ProjectConfig +from tach.extension import ProjectConfig @pytest.fixture @@ -41,28 +41,6 @@ def test_execute_with_config(capfd, mock_check, mock_project_config): assert "All modules validated!" in captured.out -def test_execute_with_error(capfd, mock_check, mock_project_config): - # Mock an error returned from check - location = Path("valid_dir/file.py") - message = "Import valid_dir in valid_dir/file.py is blocked by boundary" - mock_diagnostic = Mock(spec=Diagnostic) - mock_diagnostic.is_error.return_value = True - mock_diagnostic.to_string.return_value = message - mock_diagnostic.pyfile_path.return_value = location - mock_diagnostic.pyline_number.return_value = 0 - mock_check.return_value = [mock_diagnostic] - with pytest.raises(SystemExit) as sys_exit: - cli.tach_check( - project_root=Path(), - project_config=mock_project_config, - exclude_paths=mock_project_config.exclude, - ) - captured = capfd.readouterr() - assert sys_exit.value.code == 1 - assert str(location) in captured.err - assert message in captured.err - - def test_invalid_command(capfd): with pytest.raises(SystemExit) as sys_exit: # Test with an invalid command diff --git a/src/cli.rs b/src/cli.rs index cb5e1c56..feaa0d75 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1,6 +1,8 @@ use std::env; use std::path::Path; +use console::Term; + #[derive(Debug, PartialEq, Eq)] enum TerminalEnvironment { Unknown, @@ -41,3 +43,53 @@ pub fn create_clickable_link(file_path: &Path, abs_path: &Path, line: &usize) -> let display_with_line = format!("{}[L{}]", file_path_str, line); format!("\x1b]8;;{}\x1b\\{}\x1b]8;;\x1b\\", link, display_with_line) } + +pub fn supports_emoji() -> bool { + let term = Term::stdout(); + term.is_term() && term.features().wants_emoji() +} + +pub fn supports_colors() -> bool { + let term = Term::stdout(); + term.is_term() && term.features().colors_supported() +} + +pub struct EmojiIcons; + +impl EmojiIcons { + pub const SUCCESS: &str = "✅"; + pub const WARNING: &str = "⚠️ "; + pub const FAIL: &str = "❌"; +} + +pub struct SimpleIcons; + +impl SimpleIcons { + pub const SUCCESS: &str = "[OK]"; + pub const WARNING: &str = "[WARN]"; + pub const FAIL: &str = "[FAIL]"; +} + +pub fn success() -> &'static str { + if supports_emoji() { + EmojiIcons::SUCCESS + } else { + SimpleIcons::SUCCESS + } +} + +pub fn warning() -> &'static str { + if supports_emoji() { + EmojiIcons::WARNING + } else { + SimpleIcons::WARNING + } +} + +pub fn fail() -> &'static str { + if supports_emoji() { + EmojiIcons::FAIL + } else { + SimpleIcons::FAIL + } +} diff --git a/src/commands/check/format.rs b/src/commands/check/format.rs new file mode 100644 index 00000000..84ff75e8 --- /dev/null +++ b/src/commands/check/format.rs @@ -0,0 +1,172 @@ +use crate::{ + cli::{create_clickable_link, fail, warning}, + diagnostics::{CodeDiagnostic, Diagnostic, DiagnosticDetails, Severity}, +}; +use std::{collections::HashMap, path::PathBuf}; + +use console::style; +use itertools::Itertools; + +#[derive(Debug, PartialEq, Eq, Ord, PartialOrd, Hash, Clone)] +enum DiagnosticGroupKind { + Other, + Configuration, + ExternalDependency, + Interface, + InternalDependency, +} + +impl From<&DiagnosticDetails> for DiagnosticGroupKind { + fn from(details: &DiagnosticDetails) -> Self { + match details { + DiagnosticDetails::Configuration(..) => Self::Configuration, + DiagnosticDetails::Code(code_diagnostic_details) => match code_diagnostic_details { + CodeDiagnostic::InvalidImport { .. } => Self::InternalDependency, + CodeDiagnostic::DeprecatedImport { .. } => Self::InternalDependency, + CodeDiagnostic::LayerViolation { .. } => Self::InternalDependency, + CodeDiagnostic::PrivateImport { .. } => Self::Interface, + CodeDiagnostic::InvalidDataTypeExport { .. } => Self::Interface, + CodeDiagnostic::UndeclaredExternalDependency { .. } => Self::ExternalDependency, + CodeDiagnostic::UnusedExternalDependency { .. } => Self::ExternalDependency, + CodeDiagnostic::UnnecessarilyIgnoredImport { .. } => Self::Other, + CodeDiagnostic::UnusedIgnoreDirective { .. } => Self::Other, + CodeDiagnostic::MissingIgnoreDirectiveReason { .. } => Self::Other, + }, + } + } +} + +#[derive(Debug)] +struct DiagnosticGroup<'a> { + kind: DiagnosticGroupKind, + severity: Severity, + header: String, + diagnostics: Vec<&'a Diagnostic>, + footer: Option, +} + +impl<'a> DiagnosticGroup<'a> { + fn new(severity: Severity, kind: DiagnosticGroupKind) -> Self { + let (header, footer) = match kind { + DiagnosticGroupKind::Configuration => (style("Configuration").red().bold(), None), + DiagnosticGroupKind::InternalDependency => { + (style("Internal Dependencies").red().bold(), None) + } + DiagnosticGroupKind::ExternalDependency => { + (style("External Dependencies").red().bold(), None) + } + DiagnosticGroupKind::Interface => (style("Interfaces").red().bold(), None), + DiagnosticGroupKind::Other => (style("Other").red().bold(), None), + }; + + Self { + kind, + severity, + header: header.to_string(), + diagnostics: vec![], + footer, + } + } + + fn add_diagnostic(&mut self, diagnostic: &'a Diagnostic) { + self.diagnostics.push(diagnostic); + } + + fn sort_diagnostics(&mut self) { + self.diagnostics.sort_by(|a, b| { + // First sort by severity (warnings first) + let severity_order = b.severity().cmp(&a.severity()); + if severity_order != std::cmp::Ordering::Equal { + return severity_order; + } + + // Then sort by file path (None first) + match (a.file_path(), b.file_path()) { + (None, None) => std::cmp::Ordering::Equal, + (None, Some(_)) => std::cmp::Ordering::Less, + (Some(_), None) => std::cmp::Ordering::Greater, + (Some(a_path), Some(b_path)) => a_path.cmp(b_path), + } + }); + } +} + +pub struct DiagnosticFormatter { + project_root: PathBuf, +} + +impl DiagnosticFormatter { + pub fn new(project_root: PathBuf) -> Self { + Self { project_root } + } + + fn format_diagnostic(&self, diagnostic: &Diagnostic) -> String { + let local_error_path = diagnostic.file_path(); + + let error_location = match local_error_path { + Some(path) => { + let absolute_error_path = self.project_root.join(path); + create_clickable_link( + path, + &absolute_error_path, + &diagnostic.line_number().unwrap(), + ) + } + None => diagnostic.severity().to_string(), + }; + + let icon = match diagnostic.severity() { + Severity::Error => fail(), + Severity::Warning => warning(), + }; + + format!( + "{} {} {} {}", + icon, + style(error_location).red().bold(), + style(":").yellow().bold(), + style(diagnostic.message()).yellow(), + ) + } + + fn format_diagnostic_group(&self, group: &mut DiagnosticGroup) -> String { + group.sort_diagnostics(); + let header = match group.severity { + Severity::Error => style(&group.header).red().bold(), + Severity::Warning => style(&group.header).yellow().bold(), + }; + let diagnostics = group + .diagnostics + .iter() + .map(|d| self.format_diagnostic(d)) + .collect::>() + .join("\n"); + + match &group.footer { + Some(footer) => format!("{}\n{}\n{}", header, diagnostics, footer), + None => format!("{}\n{}", header, diagnostics), + } + } + + pub fn format_diagnostics(&self, diagnostics: &[Diagnostic]) -> String { + let mut groups: HashMap = HashMap::new(); + + for diagnostic in diagnostics { + let group_kind = DiagnosticGroupKind::from(diagnostic.details()); + let group = groups + .entry(group_kind.clone()) + .or_insert_with(|| DiagnosticGroup::new(diagnostic.severity(), group_kind)); + group.add_diagnostic(diagnostic); + } + + let mut formatted_diagnostics = Vec::new(); + for group in groups + .values_mut() + .sorted_by_key(|group| group.kind.clone()) + { + formatted_diagnostics.push(self.format_diagnostic_group(group)); + } + + formatted_diagnostics.join("\n\n") + } +} diff --git a/src/commands/check/formatter.rs b/src/commands/check/formatter.rs deleted file mode 100644 index 4f69a006..00000000 --- a/src/commands/check/formatter.rs +++ /dev/null @@ -1,3 +0,0 @@ -pub struct DiagnosticFormatter { - pub diagnostics: Vec, -} diff --git a/src/commands/check/mod.rs b/src/commands/check/mod.rs index 26df0b2d..70bfcdaf 100644 --- a/src/commands/check/mod.rs +++ b/src/commands/check/mod.rs @@ -2,6 +2,7 @@ pub mod check_external; pub mod check_internal; pub mod checks; pub mod error; +pub mod format; pub use check_external::check as check_external; pub use check_internal::check as check_internal; diff --git a/src/diagnostics.rs b/src/diagnostics.rs index b0a01ef4..3199ae61 100644 --- a/src/diagnostics.rs +++ b/src/diagnostics.rs @@ -6,13 +6,22 @@ use thiserror::Error; use crate::config::RuleSetting; -#[derive(Debug, Clone, Serialize, PartialEq)] +#[derive(Debug, Clone, Eq, PartialOrd, Ord, Serialize, PartialEq)] #[pyclass(eq, eq_int, module = "tach.extension")] pub enum Severity { Error, Warning, } +impl Display for Severity { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Severity::Error => write!(f, "Error"), + Severity::Warning => write!(f, "Warning"), + } + } +} + impl TryFrom<&RuleSetting> for Severity { type Error = (); @@ -274,6 +283,10 @@ impl Diagnostic { } } + pub fn message(&self) -> String { + self.details().to_string() + } + pub fn severity(&self) -> Severity { match self { Self::Global { severity, .. } => severity.clone(), @@ -361,10 +374,7 @@ impl Diagnostic { #[pyo3(name = "to_string")] pub fn to_pystring(&self) -> String { - match self { - Self::Global { details, .. } => details.to_string(), - Self::Located { details, .. } => details.to_string(), - } + self.message() } pub fn pyfile_path(&self) -> Option { diff --git a/src/lib.rs b/src/lib.rs index 055e59cd..d5cb649d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -350,6 +350,14 @@ fn check_internal( ) } +#[pyfunction] +pub fn format_diagnostics( + project_root: PathBuf, + diagnostics: Vec, +) -> String { + check::format::DiagnosticFormatter::new(project_root).format_diagnostics(&diagnostics) +} + #[pyfunction] #[pyo3(signature = (project_root, project_config, exclude_paths))] fn detect_unused_dependencies( @@ -409,6 +417,7 @@ fn extension(_py: Python, m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_function(wrap_pyfunction_bound!(update_computation_cache, m)?)?; m.add_function(wrap_pyfunction_bound!(dump_project_config_to_toml, m)?)?; m.add_function(wrap_pyfunction_bound!(check_internal, m)?)?; + m.add_function(wrap_pyfunction_bound!(format_diagnostics, m)?)?; m.add_function(wrap_pyfunction_bound!(detect_unused_dependencies, m)?)?; m.add_function(wrap_pyfunction_bound!(sync_project, m)?)?; m.add_function(wrap_pyfunction_bound!(run_server, m)?)?;