From 880812f9a2266d9931c4a98744d66b056f5603fa Mon Sep 17 00:00:00 2001 From: psteinroe Date: Sun, 26 Oct 2025 16:06:26 +0100 Subject: [PATCH 1/4] chore: prepare plan for refactoring --- PLAN.md | 968 +++++++++++++++++++++++++++++ crates/pgt_cli/src/commands/mod.rs | 16 +- crates/pgt_cli/src/execute/mod.rs | 5 - 3 files changed, 981 insertions(+), 8 deletions(-) create mode 100644 PLAN.md diff --git a/PLAN.md b/PLAN.md new file mode 100644 index 000000000..9a473d227 --- /dev/null +++ b/PLAN.md @@ -0,0 +1,968 @@ +# CLI Architecture Refactor Plan + +## Current Problems + +### 1. CommandRunner Trait Forces All Commands Through Same Pipeline +```rust +pub(crate) trait CommandRunner: Sized { + fn run(&mut self, session: CliSession, cli_options: &CliOptions) -> Result<()> { + // Forces: config loading → VCS setup → file traversal → reporting + let (execution, paths) = self.configure_workspace(fs, console, workspace, cli_options)?; + execute_mode(execution, session, cli_options, paths) + } +} +``` + +**Issues:** +- Only `Check` command uses this trait +- Commands like `version`, `clean`, `init` are ad-hoc functions +- No way for commands to skip parts they don't need (e.g., dblint needs config but not traversal) +- Forces unnecessary complexity on simple commands + +### 2. TraversalMode::Dummy Exists For Wrong Reasons +```rust +pub enum TraversalMode { + Dummy, // Only exists because commands are forced through traversal + Check { stdin: Option, vcs_targeted: VcsTargeted }, +} +``` + +Commands that don't need traversal shouldn't have a "dummy" mode - they just shouldn't traverse. + +### 3. Execution Struct Conflates Concerns +```rust +pub struct Execution { + report_mode: ReportMode, // How to report (Terminal/GitHub/GitLab) + traversal_mode: TraversalMode, // How to process files + max_diagnostics: u32, +} +``` + +**Problem:** Bundles processing config with reporting config. Commands that don't traverse (like `dblint`) still need reporting but don't need traversal config. + +### 4. execute_mode() is Monolithic +```rust +pub fn execute_mode(execution: Execution, session: CliSession, ...) -> Result<()> { + // Does: stdin handling + traversal + reporting + exit codes + // Can't be reused by commands that don't traverse +} +``` + +### 5. Scattered and Inconsistent Structure +- `commands/` has mix of trait impls and functions +- `execute/` has traversal logic tightly coupled to reporting +- No clear separation of workspace setup, execution, and reporting +- Commands repeat boilerplate for config loading, setup, reporting + +--- + +## Proposed Clean Architecture + +### Three Command Tiers + +We have three distinct types of commands: + +**Tier 1: Simple** - No workspace needed +- `version`, `clean`, `stop`, `print_socket` +- Just need Console/FileSystem + +**Tier 2: Workspace-only** - Config/workspace, no file traversal +- `init`, `dblint` +- Load config, setup workspace, analyze, report + +**Tier 3: Full Pipeline** - Config + workspace + file traversal +- `check`, (future: `format`, `lint`) +- Load config, setup workspace with VCS, traverse files, report + +### New Architecture Layers + +``` +┌─────────────────────────────────────────┐ +│ commands/ │ +│ Simple functions, no forced trait │ +│ - version, clean, init, dblint, check │ +└─────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────┐ +│ CliSession with Helper Methods │ +│ - prepare_with_config() │ +│ - setup_workspace(VcsIntegration) │ +│ - report() │ +└─────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────┐ +│ workspace.rs (NEW) │ +│ - load_config() │ +│ - setup_workspace(..., VcsIntegration) │ +└─────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────┐ +│ execute/ (RESTRUCTURED) │ +│ - ExecutionConfig + helpers │ +│ - run_files() │ +│ - run_stdin() │ +│ Clear split between walker & stdin │ +└─────────────────────────────────────────┘ + ↓ +┌─────────────────────────────────────────┐ +│ reporter/ │ +│ - Reporter struct + Report enum │ +│ Works with ANY diagnostic source │ +└─────────────────────────────────────────┘ +``` + +--- + +## Detailed Design + +### 1. CliSession Helper Methods + +Add helper methods to reduce boilerplate: + +```rust +// crates/pgt_cli/src/lib.rs + +impl<'app> CliSession<'app> { + /// Helper accessors + pub fn fs(&self) -> &DynRef<'_, dyn FileSystem> { + &self.app.fs + } + + pub fn console(&mut self) -> &mut dyn Console { + &mut *self.app.console + } + + pub fn workspace(&self) -> &dyn Workspace { + &*self.app.workspace + } + + /// Common setup: logging + config loading + merging + pub fn prepare_with_config( + &mut self, + cli_options: &CliOptions, + cli_configuration: Option, + ) -> Result { + setup_cli_subscriber(cli_options.log_level, cli_options.log_kind); + + // Take borrows once so immutable/mutable access do not overlap + let fs = self.fs(); + let console = self.console(); + let loaded = workspace::load_config( + fs, + console, + cli_options.as_configuration_path_hint(), + )?; + + let mut configuration = loaded.configuration; + if let Some(cli_config) = cli_configuration { + configuration.merge_with(cli_config); + } + + Ok(configuration) + } + + /// Setup workspace with optional VCS integration + pub fn setup_workspace( + &mut self, + configuration: PartialConfiguration, + vcs: VcsIntegration, + ) -> Result<(), CliDiagnostic> { + let workspace = self.workspace(); + let fs = self.fs(); + + workspace::setup_workspace(workspace, fs, configuration, vcs) + } + + /// Report results (common final step) + pub fn report( + &mut self, + cli_options: &CliOptions, + report: Report, + ) -> Result<(), CliDiagnostic> { + let mut reporter = Reporter::from_cli_options(cli_options); + reporter.report(self.console(), report) + } +} + +/// Controls whether workspace setup includes VCS integration +pub enum VcsIntegration { + /// Enable VCS integration (gitignore, changed files detection) + Enabled, + /// Skip VCS integration + Disabled, +} +} +``` + +### 2. Workspace Setup Layer (NEW) + +```rust +// crates/pgt_cli/src/workspace.rs - NEW FILE + +use crate::VcsIntegration; +use pgt_configuration::PartialConfiguration; +use pgt_console::{Console, ConsoleExt, markup}; +use pgt_fs::{ConfigName, FileSystem}; +use pgt_workspace::{DynRef, Workspace}; +use pgt_workspace::configuration::{LoadedConfiguration, load_configuration}; +use pgt_workspace::workspace::{RegisterProjectFolderParams, UpdateSettingsParams}; +use std::path::PathBuf; + +/// Load configuration from filesystem with deprecation warning +pub fn load_config( + fs: &DynRef<'_, dyn FileSystem>, + console: &mut dyn Console, + config_hint: Option<&PathBuf>, +) -> Result { + let loaded = load_configuration(fs, config_hint)?; + + if let Some(config_path) = &loaded.file_path { + if let Some(file_name) = config_path.file_name().and_then(|n| n.to_str()) { + if ConfigName::is_deprecated(file_name) { + console.log(markup! { + "Warning: ""Deprecated config filename. Use 'postgres-language-server.jsonc'.\n" + }); + } + } + } + + Ok(loaded) +} + +pub fn setup_workspace( + workspace: &dyn Workspace, + fs: &DynRef<'_, dyn FileSystem>, + configuration: PartialConfiguration, + vcs: VcsIntegration, +) -> Result<(), CliDiagnostic> { + let (vcs_base_path, gitignore_matches) = match vcs { + VcsIntegration::Enabled => configuration + .retrieve_gitignore_matches(fs, fs.working_directory().as_deref()) + .map_err(CliDiagnostic::from)?, + VcsIntegration::Disabled => (None, vec![]), + }; + + workspace + .register_project_folder(RegisterProjectFolderParams { + path: fs.working_directory(), + set_as_current_workspace: true, + }) + .map_err(CliDiagnostic::from)?; + + workspace + .update_settings(UpdateSettingsParams { + workspace_directory: fs.working_directory(), + configuration, + vcs_base_path, + gitignore_matches, + }) + .map_err(CliDiagnostic::from)?; + + Ok(()) +} +``` + +### 3. execute/ Module (restructured, not new) + +We keep the familiar `execute/` name but make the responsibilities crystal clear: + +```text +execute/ + mod.rs - public entry points (run_files, run_stdin) + ExecutionConfig + config.rs - ExecutionMode / FixMode / VcsTargeting helpers + walk.rs - filesystem traversal + VCS-aware path filtering + stdin.rs - converts stdin payload into process_file jobs + process_file/ - unchanged, per-file logic +``` + +#### Execution configuration lives in `execute/config.rs` + +```rust +// Shared across files + stdin paths +pub struct ExecutionConfig { + pub mode: ExecutionMode, + pub limits: ExecutionLimits, +} + +pub enum ExecutionMode { + Check { fix_mode: Option, vcs: VcsTargeting }, + Format { write: bool, ignore_errors: bool, vcs: VcsTargeting }, + Lint { only: Vec, skip: Vec, fix_mode: Option, vcs: VcsTargeting }, +} + +pub enum FixMode { + Safe, + Suggested, +} + +pub struct VcsTargeting { + pub staged: bool, + pub changed: bool, +} + +pub struct ExecutionLimits { + pub max_diagnostics: u32, + pub allow_writes: bool, +} + +impl ExecutionConfig { + pub fn new(mode: ExecutionMode, max_diagnostics: u32) -> Self { + let allow_writes = matches!( + &mode, + ExecutionMode::Check { fix_mode: Some(_), .. } + | ExecutionMode::Format { write: true, .. } + | ExecutionMode::Lint { fix_mode: Some(_), .. } + ); + Self { + mode, + limits: ExecutionLimits { + max_diagnostics, + allow_writes, + }, + } + } +} +``` + +Having a single config object means commands share one type regardless of whether they operate on stdin or directories. `allow_writes` is derived from the mode so CLI commands can gate destructive actions early. + +#### Public helpers stay tiny + +```rust +pub fn run_files( + session: &mut CliSession, + config: &ExecutionConfig, + paths: Vec, +) -> Result { + walk::traverse(session, config, paths) +} + +pub fn run_stdin( + session: &mut CliSession, + config: &ExecutionConfig, + payload: StdinPayload, +) -> Result { + stdin::process(session, config, payload) +} + +pub struct StdinPayload { + pub path: PathBuf, + pub content: String, +} +``` + +Where `StdinPayload` is a simple `{ path: PathBuf, content: String }` struct defined next to the helpers. + +#### Reporting structures stay outside execute/ + +`execute/` no longer defines the old `DiagnosticResult` / `TraversalResult` structs. Instead it emits neutral `ExecutionSummary` values (counts, changed/skipped, collected diagnostics). The reusable `DiagnosticSummary` type that dblint also needs lives under `reporter/summary.rs` (see next section). This keeps execute/ focused on producing data, not on how it will be displayed. + +```rust +// crates/pgt_cli/src/reporter/mod.rs - ADD REPORT CONFIG + +/// Configuration for how to report results +/// SEPARATE from traversal configuration +#[derive(Debug, Clone)] +pub struct ReportConfig { + pub mode: ReportMode, + pub verbose: bool, + pub diagnostic_level: DiagnosticLevel, + pub error_on_warnings: bool, + pub no_errors_on_unmatched: bool, +} + +impl ReportConfig { + pub fn from_cli_options(cli_options: &CliOptions) -> Self { + Self { + mode: cli_options.reporter.clone().into(), + verbose: cli_options.verbose, + diagnostic_level: cli_options.diagnostic_level, + error_on_warnings: cli_options.error_on_warnings, + no_errors_on_unmatched: cli_options.no_errors_on_unmatched, + } + } +} + +/// Summaries live with the reporter so non-traversal commands (dblint) can reuse them +pub struct DiagnosticSummary { + pub diagnostics: Vec, + pub duration: Duration, + pub errors: u32, + pub warnings: u32, +} + +impl DiagnosticSummary { + pub fn from_diagnostics(diagnostics: Vec, duration: Duration) -> Self { + let errors = diagnostics.iter().filter(|d| d.severity().is_error()).count() as u32; + let warnings = diagnostics.iter().filter(|d| d.severity().is_warning()).count() as u32; + Self { diagnostics, duration, errors, warnings } + } +} + +pub struct ExecutionSummary { + pub diagnostics: DiagnosticSummary, + pub evaluated_paths: Vec, + pub changed: usize, + pub unchanged: usize, + pub skipped: usize, +} + +/// User-facing type describing what should be reported +pub enum Report { + Diagnostics { + summary: DiagnosticSummary, + command_name: &'static str, + }, + Traversal { + summary: ExecutionSummary, + command_name: &'static str, + }, +} + +impl Report { + pub fn from_diagnostic_summary( + command_name: &'static str, + summary: DiagnosticSummary, + ) -> Self { + Report::Diagnostics { summary, command_name } + } + + pub fn from_execution_summary( + command_name: &'static str, + summary: ExecutionSummary, + ) -> Self { + Report::Traversal { summary, command_name } + } +} + +/// Reporter struct instead of bare function calls +pub struct Reporter { + config: ReportConfig, +} + +impl Reporter { + pub fn from_cli_options(cli_options: &CliOptions) -> Self { + Self { + config: ReportConfig::from_cli_options(cli_options), + } + } + + pub fn report( + &mut self, + console: &mut dyn Console, + report: Report, + ) -> Result<(), CliDiagnostic> { + match report { + Report::Diagnostics { summary, command_name } => { + self.report_diagnostics(console, summary, command_name) + } + Report::Traversal { summary, command_name } => { + self.report_traversal(console, summary, command_name) + } + } + } + + fn report_diagnostics( + &self, + console: &mut dyn Console, + summary: DiagnosticSummary, + command_name: &str, + ) -> Result<(), CliDiagnostic> { + // existing logic from report_analysis + } + + fn report_traversal( + &self, + console: &mut dyn Console, + summary: ExecutionSummary, + command_name: &str, + ) -> Result<(), CliDiagnostic> { + // existing logic from report_traversal + } +} + +// Implementation note: the bodies of `report_diagnostics` and `report_traversal` +// reuse today's `report_analysis` / `report_traversal` logic verbatim, just moved +// behind the struct methods so callers always use the same entry point. +``` + +### 4. Example Commands + +#### Tier 1: Simple Command (No Config) + +```rust +// crates/pgt_cli/src/commands/version.rs + +pub fn version(mut session: CliSession) -> Result<(), CliDiagnostic> { + session.console().log(markup! { + "CLI: "{VERSION} + }); + + match session.workspace().server_info() { + None => { + session.console().log(markup! { + "Server: ""not connected" + }); + } + Some(info) => { + session.console().log(markup! { + "Server: + Name: "{info.name}" + Version: "{info.version.unwrap_or_else(|| "-".to_string())} + }); + } + }; + + Ok(()) +} +``` + +#### Tier 2: Config-Only Command (No Traversal) + +```rust +// crates/pgt_cli/src/commands/dblint.rs + +pub fn dblint( + mut session: CliSession, + cli_options: &CliOptions, + cli_configuration: Option, +) -> Result<(), CliDiagnostic> { + // Step 1: Common setup (logging + config loading + merging) + let configuration = session.prepare_with_config(cli_options, cli_configuration)?; + + // Step 2: Setup workspace (no VCS needed) + session.setup_workspace(configuration.clone(), VcsIntegration::Disabled)?; + + // Step 3: Custom analysis logic (no file traversal!) + let start = Instant::now(); + let diagnostics = analyze_workspace_config(session.workspace(), &configuration)?; + let duration = start.elapsed(); + + // Step 4: Build summary (clean - no fake traversal data!) + let summary = DiagnosticSummary::from_diagnostics(diagnostics, duration); + + // Step 5: Report (same infrastructure as check) + session.report( + cli_options, + Report::from_diagnostic_summary("dblint", summary), + ) +} + +fn analyze_workspace_config( + workspace: &dyn Workspace, + config: &PartialConfiguration, +) -> Result, CliDiagnostic> { + // Your dblint analysis logic here + // Example: validate linting rules, check for conflicts, etc. + let mut diagnostics = vec![]; + + if let Some(linter_config) = &config.linter { + if linter_config.rules.is_empty() { + diagnostics.push( + Error::from_message("No linting rules configured") + .with_severity(Severity::Warning) + .with_category(category!("dblint")) + ); + } + } + + Ok(diagnostics) +} + +``` + +#### Tier 3: Full Pipeline Command (Traversal) + +```rust +// crates/pgt_cli/src/commands/check.rs + +pub struct CheckArgs { + pub configuration: Option, + pub paths: Vec, + pub stdin_file_path: Option, + pub staged: bool, + pub changed: bool, + pub since: Option, + pub apply: bool, + pub apply_unsafe: bool, +} + +pub fn check( + mut session: CliSession, + cli_options: &CliOptions, + args: CheckArgs, +) -> Result<(), CliDiagnostic> { + // Step 1: Common setup + let configuration = session.prepare_with_config(cli_options, args.configuration)?; + validate_args(&args)?; + + // Step 2: Setup workspace with VCS (needed for traversal) + session.setup_workspace(configuration.clone(), VcsIntegration::Enabled)?; + + // Step 3: Compute paths (from CLI args, VCS, or default) + let paths = compute_paths(session.fs(), &configuration, &args)?; + + // Step 4: Build execution mode + let mode = ExecutionMode::Check { + fix_mode: if args.apply_unsafe { + Some(FixMode::Suggested) + } else if args.apply { + Some(FixMode::Safe) + } else { + None + }, + vcs: VcsTargeting { + staged: args.staged, + changed: args.changed, + }, + }; + + // Step 5: Build config once and reuse it regardless of the input source + let max_diagnostics = if cli_options.reporter.is_default() { + cli_options.max_diagnostics + } else { + u32::MAX + }; + let execution_config = ExecutionConfig::new(mode, max_diagnostics); + + let summary = if let Some(stdin_path) = args.stdin_file_path { + let content = session.console().read() + .ok_or_else(|| CliDiagnostic::missing_argument("stdin", "check"))?; + + execute::run_stdin( + &mut session, + &execution_config, + StdinPayload { + path: PathBuf::from(stdin_path), + content, + }, + )? + } else { + execute::run_files(&mut session, &execution_config, paths)? + }; + + // Step 6: Report + session.report( + cli_options, + Report::from_execution_summary("check", summary), + ) +} + +fn validate_args(args: &CheckArgs) -> Result<(), CliDiagnostic> { + if args.since.is_some() && !args.changed { + return Err(CliDiagnostic::incompatible_arguments("since", "changed")); + } + if args.changed && args.staged { + return Err(CliDiagnostic::incompatible_arguments("changed", "staged")); + } + Ok(()) +} + +fn compute_paths( + fs: &DynRef<'_, dyn FileSystem>, + configuration: &PartialConfiguration, + args: &CheckArgs, +) -> Result, CliDiagnostic> { + if args.changed { + get_changed_files(fs, configuration, args.since.as_deref()) + } else if args.staged { + get_staged_files(fs) + } else if !args.paths.is_empty() { + Ok(args.paths.clone()) + } else { + Ok(vec![]) // Default to current directory (handled inside execute::run_files) + } +} +``` + +### 5. Execution Flow + +Here's how the "full pipeline" path works in the check command: + +``` +check command + ↓ +execute::run_files(&mut session, &config, paths) + ↓ +walk::traverse(session, config, paths) + ↓ +Walk file system from starting paths + ↓ +For each file found: + ↓ +process_file::process_file(options, path) + ↓ (dispatches based on mode) + ↓ +For Check mode: + workspace.pull_diagnostics() - runs linting +For Format mode: + workspace.format() - formats file +For Lint mode: + workspace.pull_diagnostics() with specific rules + ↓ +Send diagnostics to collector thread + ↓ +After traversal completes: + ↓ +Return ExecutionSummary { diagnostics, counts } + ↓ +check command → Reporter::report(...) +``` + +**Key insight:** The `execute/` module is **mode-agnostic** - it works with Check, Format, Lint, etc. Commands like `dblint`, `version`, `init` never import it because they don't process files. + +### 6. Updated lib.rs + +```rust +// crates/pgt_cli/src/lib.rs + +mod cli_options; +mod commands; +mod diagnostics; +mod logging; +mod reporter; +mod execute; // RESTRUCTURED - replaces old execute/ +mod workspace; // NEW + +impl<'app> CliSession<'app> { + // ... helper methods defined above ... + + pub fn run(self, command: PgtCommand) -> Result<(), CliDiagnostic> { + match command { + PgtCommand::Version(_) => commands::version::version(self), + PgtCommand::Clean => commands::clean::clean(self), + PgtCommand::Init => commands::init::init(self), + + PgtCommand::Dblint { cli_options, configuration } => { + commands::dblint::dblint(self, &cli_options, configuration) + } + + PgtCommand::Check { + cli_options, + configuration, + paths, + stdin_file_path, + staged, + changed, + since, + } => { + commands::check::check( + self, + &cli_options, + commands::check::CheckArgs { + configuration, + paths, + stdin_file_path, + staged, + changed, + since, + apply: false, // Add these flags to PgtCommand enum + apply_unsafe: false, + }, + ) + } + + // ... other commands + } + } +} + +// REMOVE: run_command() helper +// REMOVE: CommandRunner trait +``` + +--- + +## Comparison: Before vs After + +| Aspect | Current | Proposed | +|--------|---------|----------| +| **Command structure** | Mix of CommandRunner trait + ad-hoc functions | All functions, no forced trait | +| **Simple commands** | Ad-hoc implementations | Clean functions using helpers | +| **Config-only commands** | No good pattern | `prepare_with_config()` + `setup_workspace(VcsIntegration::Disabled)` | +| **Workspace setup** | `setup_full_workspace()` vs `setup_minimal_workspace()` (unclear) | `setup_workspace(config, VcsIntegration::Enabled/Disabled)` (clear) | +| **Result types** | TraversalSummary used even when not traversing | `DiagnosticSummary` for analysis, `ExecutionSummary` for traversal | +| **Traversal commands** | Forced through CommandRunner | Explicit `execute::run_files()` call | +| **Code reuse** | Trait with mandatory methods | Helper methods on CliSession | +| **Traversal modes** | Has Dummy mode | No Dummy - traversal is opt-in | +| **Execution bundle** | Conflates processing + reporting | Split: ExecutionConfig + ReportConfig | +| **Reporting** | Coupled to execute_mode | `Reporter` struct with `Report` payloads | +| **Boilerplate** | High - repeated config loading | Low - helpers reduce repetition | +| **Flexibility** | Low - forced through pipeline | High - compose what you need | +| **Clarity** | Unclear what commands need | Crystal clear from function calls | + +--- + +## Pros and Cons + +### Pros + +✅ **Clear command tiers** - Easy to understand what each command needs +✅ **No forced abstractions** - Commands use only what they need +✅ **ExecutionMode preserved** - Keeps rich configuration like Biome +✅ **No Dummy mode** - Traversal is truly optional +✅ **Separation of concerns** - Processing config separate from reporting config +✅ **Reusable infrastructure** - Reporting works for all commands +✅ **Reduced boilerplate** - Helper methods eliminate repetition +✅ **Extensible** - Easy to add new modes (Format, Lint) or commands +✅ **Testable** - Each layer can be tested independently +✅ **Explicit data flow** - No hidden trait magic +✅ **Flexible reporting** - Any command can generate diagnostics and use same reporting +✅ **Clear workspace setup** - `VcsIntegration::Enabled/Disabled` is self-documenting +✅ **Proper result types** - DiagnosticSummary vs ExecutionSummary - no fake data + +### Cons + +⚠️ **More methods on CliSession** - Could become bloated if too many helpers added +⚠️ **No enforced structure** - Commands could skip important steps if not careful +⚠️ **Duplication risk** - Without trait, commands might duplicate logic (mitigated by helpers) +⚠️ **Learning curve** - New contributors need to understand helper methods +⚠️ **No compile-time guarantees** - Can't enforce "must setup workspace before traversal" + +### Mitigation Strategies + +- Keep CliSession helpers focused on truly common patterns +- Document command implementation patterns clearly +- Use helper function modules (like workspace.rs) for shared logic +- Add integration tests that verify correct command flow +- Add focused helper functions (e.g., `ensure_workspace_ready`) if we need stronger guidance later + +--- + +## Migration Path + +### Phase 1: Setup Foundation +1. Create `crates/pgt_cli/src/workspace.rs` with helper functions +2. Add helper methods to `CliSession` (accessors + borrow-safe `prepare_with_config` + `setup_workspace`) +3. Split `Execution` into `ExecutionConfig` and `ReportConfig` +4. Introduce `Reporter` struct (`from_cli_options` + `report()`) + +### Phase 2: Migrate Simple Commands +1. Refactor `version` to use new helpers (already simple) +2. Refactor `clean` to use new helpers +3. Smoke-test both commands + +### Phase 3: Add Dblint (Config-Only Command) +1. Implement `commands/dblint.rs` using `prepare_with_config()` + `setup_workspace(...Disabled)` +2. Implement `analyze_workspace_config()` logic +3. Add dblint to `PgtCommand` enum and `CliSession::run()` +4. Test dblint command + +### Phase 4: Migrate Check Command +1. Restructure `execute/` (config.rs, walk.rs, stdin.rs, process_file/) +2. Add `execute::run_files()` helper (thin wrapper over walk) +3. Add `execute::run_stdin()` helper (buffer path) +4. Update `walk.rs` to use `ExecutionConfig` +5. Ensure walker no longer depends on stdin specifics +6. Refactor `check.rs` to call `run_stdin`/`run_files` +7. Test check command thoroughly (traversal, stdin, VCS modes) + +### Phase 5: Cleanup +1. Remove `CommandRunner` trait from `commands/mod.rs` +2. Remove `execute_mode()` function +3. Remove `TraversalMode::Dummy` +4. Remove old `Execution` struct +5. Update imports to new `execute::{self, config, stdin}` layout +6. Update/extend tests +7. Update documentation + +### Phase 6: Future Commands +1. Add `format` command using ExecutionMode::Format +2. Add `lint` command using ExecutionMode::Lint +3. Each new command follows established patterns + +--- + +## File Changes Summary + +### Files to Create/Rename +- `crates/pgt_cli/src/workspace.rs` - NEW - Workspace setup helpers +- `crates/pgt_cli/src/commands/dblint.rs` - NEW - Dblint command + +### Files to Modify (Significant) +- `crates/pgt_cli/src/lib.rs` - Add helper methods to CliSession, simplify run() +- `crates/pgt_cli/src/execute/mod.rs` - Export `ExecutionConfig`, `run_files`, `run_stdin` +- `crates/pgt_cli/src/execute/config.rs` - NEW internal module for modes/limits +- `crates/pgt_cli/src/execute/walk.rs` - Rewritten to use `ExecutionConfig` +- `crates/pgt_cli/src/execute/stdin.rs` - Buffer path feeding process_file +- `crates/pgt_cli/src/reporter/mod.rs` - Add ReportConfig, `Reporter` struct, `Report` enum, summaries +- `crates/pgt_cli/src/commands/check.rs` - Call `run_stdin`/`run_files` +- `crates/pgt_cli/src/commands/mod.rs` - Remove CommandRunner trait (~135 lines) +- `crates/pgt_cli/src/execute/process_file/mod.rs` - Dispatch based on ExecutionMode + +### Files to Modify (Minor) +- `crates/pgt_cli/src/commands/version.rs` - Use helper methods +- `crates/pgt_cli/src/commands/clean.rs` - Use helper methods +- `crates/pgt_cli/src/execute/process_file/*.rs` - Use ExecutionMode from config +- Remove the old `execute/std_in.rs` and fold logic into `execute/stdin.rs` + +### Lines of Code Impact +- **Delete:** ~200 lines (CommandRunner trait, execute_mode monolith, Dummy mode) +- **Add:** ~300 lines (workspace.rs, helpers, dblint, configs) +- **Net:** +100 lines but much clearer structure + +--- + +## Key Design Principles + +1. **Opt-in over forced** - Commands choose what they need, not forced through pipeline +2. **Separation of concerns** - Config, traversal, reporting are independent +3. **Explicit over implicit** - Function calls show what's happening, no hidden trait magic +4. **Simple stays simple** - Don't force complexity on commands that don't need it +5. **Reusable infrastructure** - Reporting works for any diagnostic source +6. **Composable helpers** - Small, focused helper methods instead of monolithic abstractions +7. **Rich configuration** - ExecutionMode captures "how" to process files, keep it flexible +8. **Clear boundaries** - `execute/` owns runtime pipeline, stdin/files are explicit helpers + +### Why keep `execute/` but reshape it? + +**Original pain:** `execute/` mixed config, traversal, stdin, and reporting glue. You had to understand everything to touch anything. + +**New structure:** +``` +execute/ + mod.rs - `ExecutionConfig`, `run_files`, `run_stdin` + config.rs - ExecutionMode + FixMode + limits + walk.rs - directory walking + VCS targeting + stdin.rs - buffer path, no filesystem assumptions + process_file/ - unchanged processing logic +``` + +Now the name still matches the CLI command (`pgt execute check`), but the responsibilities are sliced: commands talk to `run_*`, the walker never sees stdin, and reporting lives elsewhere. + +--- + +## Open Questions + +1. **Should we add more optional helpers?** e.g., `compute_vcs_paths()`, `read_stdin()` + - Lean towards yes, but only after seeing repeated patterns + +2. **Should ReportConfig handle category name or commands pass it?** + - Currently commands pass command_name - keeps reporting generic + +3. **Do we need a WorkspaceConfig struct?** + - Currently passing PartialConfiguration directly - works but could bundle with paths + +4. **Should stdin handling stay inside execute/?** + - Current plan keeps `run_stdin()` next to `run_files()` for shared logic, but we can split later if it grows complex + +--- + +## Success Criteria + +✅ All three command tiers have clear, distinct implementations +✅ No TraversalMode::Dummy +✅ Dblint command works without traversal +✅ Check command works with traversal (files, stdin, VCS modes) +✅ All reporters (Terminal, GitHub, GitLab, JUnit) work for all commands +✅ Less boilerplate in command implementations +✅ All existing tests pass +✅ Code is easier to understand for new contributors + +--- + +## References + +- Biome CLI architecture - Good example of rich execution modes +- Current `pgt_cli` implementation - Understanding existing patterns +- Rust API guidelines - General API ergonomics and error handling guidance diff --git a/crates/pgt_cli/src/commands/mod.rs b/crates/pgt_cli/src/commands/mod.rs index d66b27ba8..043660be9 100644 --- a/crates/pgt_cli/src/commands/mod.rs +++ b/crates/pgt_cli/src/commands/mod.rs @@ -30,6 +30,16 @@ pub enum PgtCommand { #[bpaf(command)] Version(#[bpaf(external(cli_options), hide_usage)] CliOptions), + /// Runs everything to the requested files. + #[bpaf(command)] + Dblint { + #[bpaf(external(partial_configuration), hide_usage, optional)] + configuration: Option, + + #[bpaf(external, hide_usage)] + cli_options: CliOptions, + }, + /// Runs everything to the requested files. #[bpaf(command)] Check { @@ -217,9 +227,9 @@ pub enum PgtCommand { impl PgtCommand { const fn cli_options(&self) -> Option<&CliOptions> { match self { - PgtCommand::Version(cli_options) | PgtCommand::Check { cli_options, .. } => { - Some(cli_options) - } + PgtCommand::Version(cli_options) + | PgtCommand::Check { cli_options, .. } + | PgtCommand::Dblint { cli_options, .. } => Some(cli_options), PgtCommand::LspProxy { .. } | PgtCommand::Start { .. } | PgtCommand::Stop diff --git a/crates/pgt_cli/src/execute/mod.rs b/crates/pgt_cli/src/execute/mod.rs index 6cb01ca7a..a8351fc98 100644 --- a/crates/pgt_cli/src/execute/mod.rs +++ b/crates/pgt_cli/src/execute/mod.rs @@ -76,11 +76,6 @@ pub enum TraversalMode { Dummy, /// This mode is enabled when running the command `check` Check { - // The type of fixes that should be applied when analyzing a file. - // - // It's [None] if the `check` command is called without `--apply` or `--apply-suggested` - // arguments. - // fix_file_mode: Option, /// An optional tuple. /// 1. The virtual path to the file /// 2. The content of the file From bd5ff585ca07501684a84594bb5337e549f92973 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Tue, 28 Oct 2025 09:30:12 +0100 Subject: [PATCH 2/4] progress --- crates/pgt_cli/src/commands/check.rs | 179 ++++++---- crates/pgt_cli/src/commands/dblint.rs | 18 + crates/pgt_cli/src/commands/mod.rs | 154 +-------- crates/pgt_cli/src/commands/version.rs | 22 +- crates/pgt_cli/src/execute/config.rs | 63 ++++ crates/pgt_cli/src/execute/mod.rs | 313 ++---------------- crates/pgt_cli/src/execute/process_file.rs | 11 +- .../execute/process_file/workspace_file.rs | 6 +- crates/pgt_cli/src/execute/std_in.rs | 11 - crates/pgt_cli/src/execute/stdin.rs | 13 + .../src/execute/{traverse.rs => walk.rs} | 211 +++--------- crates/pgt_cli/src/lib.rs | 102 +++++- crates/pgt_cli/src/reporter/github.rs | 51 ++- crates/pgt_cli/src/reporter/gitlab.rs | 118 +++---- crates/pgt_cli/src/reporter/junit.rs | 184 +++++----- crates/pgt_cli/src/reporter/mod.rs | 154 +++++++-- crates/pgt_cli/src/reporter/terminal.rs | 249 +++++++------- crates/pgt_cli/src/workspace.rs | 48 +++ ..._check__check_junit_reporter_snapshot.snap | 6 +- 19 files changed, 847 insertions(+), 1066 deletions(-) create mode 100644 crates/pgt_cli/src/commands/dblint.rs create mode 100644 crates/pgt_cli/src/execute/config.rs delete mode 100644 crates/pgt_cli/src/execute/std_in.rs create mode 100644 crates/pgt_cli/src/execute/stdin.rs rename crates/pgt_cli/src/execute/{traverse.rs => walk.rs} (66%) create mode 100644 crates/pgt_cli/src/workspace.rs diff --git a/crates/pgt_cli/src/commands/check.rs b/crates/pgt_cli/src/commands/check.rs index 468196244..4d00b9adf 100644 --- a/crates/pgt_cli/src/commands/check.rs +++ b/crates/pgt_cli/src/commands/check.rs @@ -1,76 +1,141 @@ use crate::cli_options::CliOptions; -use crate::{CliDiagnostic, Execution, TraversalMode}; -use biome_deserialize::Merge; +use crate::commands::get_files_to_process_with_cli_options; +use crate::execute::{StdinPayload, run_files, run_stdin}; +use crate::reporter::Report; +use crate::{CliDiagnostic, CliSession, VcsIntegration}; +use crate::{ExecutionConfig, ExecutionMode, VcsTargeting}; use pgt_configuration::PartialConfiguration; use pgt_console::Console; +use pgt_diagnostics::category; use pgt_fs::FileSystem; -use pgt_workspace::{DynRef, Workspace, WorkspaceError, configuration::LoadedConfiguration}; +use pgt_workspace::DynRef; use std::ffi::OsString; -use super::{CommandRunner, get_files_to_process_with_cli_options}; +#[allow(dead_code)] +pub struct CheckArgs { + pub configuration: Option, + pub paths: Vec, + pub stdin_file_path: Option, + pub staged: bool, + pub changed: bool, + pub since: Option, + pub apply: bool, + pub apply_unsafe: bool, +} + +pub fn check( + mut session: CliSession, + cli_options: &CliOptions, + args: CheckArgs, +) -> Result<(), CliDiagnostic> { + validate_args(&args)?; + + let configuration = session.prepare_with_config(cli_options, args.configuration.clone())?; + session.setup_workspace(configuration.clone(), VcsIntegration::Enabled)?; + + let paths = resolve_paths(session.fs(), &configuration, &args)?; + + let vcs = VcsTargeting { + staged: args.staged, + changed: args.changed, + }; + + let max_diagnostics = if cli_options.reporter.is_default() { + cli_options.max_diagnostics.into() + } else { + u32::MAX + }; + + let mode = ExecutionMode::Check { vcs }; + let execution = ExecutionConfig::new(mode, max_diagnostics); -pub(crate) struct CheckCommandPayload { - pub(crate) configuration: Option, - pub(crate) paths: Vec, - pub(crate) stdin_file_path: Option, - pub(crate) staged: bool, - pub(crate) changed: bool, - pub(crate) since: Option, + if let Some(stdin_path) = args.stdin_file_path.as_deref() { + let payload = read_stdin_payload(stdin_path, session.console())?; + run_stdin(&mut session, &execution, payload) + } else { + let report: Report = run_files(&mut session, &execution, paths)?; + + let exit_result = enforce_exit_codes(cli_options, &report); + session.report("check", cli_options, &report)?; + exit_result + } } -impl CommandRunner for CheckCommandPayload { - const COMMAND_NAME: &'static str = "check"; - - fn merge_configuration( - &mut self, - loaded_configuration: LoadedConfiguration, - _fs: &DynRef<'_, dyn FileSystem>, - _console: &mut dyn Console, - ) -> Result { - let LoadedConfiguration { - configuration: mut fs_configuration, - .. - } = loaded_configuration; - - if let Some(configuration) = self.configuration.clone() { - // overwrite fs config with cli args - fs_configuration.merge_with(configuration); +fn resolve_paths( + fs: &DynRef<'_, dyn FileSystem>, + configuration: &PartialConfiguration, + args: &CheckArgs, +) -> Result, CliDiagnostic> { + let mut paths = get_files_to_process_with_cli_options( + args.since.as_deref(), + args.changed, + args.staged, + fs, + configuration, + )? + .unwrap_or_else(|| args.paths.clone()); + + if paths.is_empty() && args.stdin_file_path.is_none() { + if let Some(current_dir) = fs.working_directory() { + paths.push(current_dir.into_os_string()); } + } - Ok(fs_configuration) + Ok(paths) +} + +fn read_stdin_payload( + path: &str, + console: &mut dyn Console, +) -> Result { + let input_code = console.read(); + if let Some(input_code) = input_code { + Ok(StdinPayload { + path: path.into(), + content: input_code, + }) + } else { + Err(CliDiagnostic::missing_argument("stdin", "check")) } +} + +fn enforce_exit_codes(cli_options: &CliOptions, payload: &Report) -> Result<(), CliDiagnostic> { + let traversal = payload.traversal.as_ref(); + let processed = traversal.map_or(0, |t| t.changed + t.unchanged); + let skipped = traversal.map_or(0, |t| t.skipped); - fn get_files_to_process( - &self, - fs: &DynRef<'_, dyn FileSystem>, - configuration: &PartialConfiguration, - ) -> Result, CliDiagnostic> { - let paths = get_files_to_process_with_cli_options( - self.since.as_deref(), - self.changed, - self.staged, - fs, - configuration, - )? - .unwrap_or(self.paths.clone()); - - Ok(paths) + if processed.saturating_sub(skipped) == 0 && !cli_options.no_errors_on_unmatched { + return Err(CliDiagnostic::no_files_processed()); } - fn get_stdin_file_path(&self) -> Option<&str> { - self.stdin_file_path.as_deref() + let warnings = payload.warnings; + let errors = payload.errors; + let category = category!("check"); + + if errors > 0 { + return Err(CliDiagnostic::check_error(category)); } - fn get_execution( - &self, - cli_options: &CliOptions, - console: &mut dyn Console, - _workspace: &dyn Workspace, - ) -> Result { - Ok(Execution::new(TraversalMode::Check { - stdin: self.get_stdin(console)?, - vcs_targeted: (self.staged, self.changed).into(), - }) - .set_report(cli_options)) + if warnings > 0 && cli_options.error_on_warnings { + return Err(CliDiagnostic::check_warnings(category)); } + + Ok(()) +} + +fn validate_args(args: &CheckArgs) -> Result<(), CliDiagnostic> { + if args.since.is_some() { + if !args.changed { + return Err(CliDiagnostic::incompatible_arguments("since", "changed")); + } + if args.staged { + return Err(CliDiagnostic::incompatible_arguments("since", "staged")); + } + } + + if args.changed && args.staged { + return Err(CliDiagnostic::incompatible_arguments("changed", "staged")); + } + + Ok(()) } diff --git a/crates/pgt_cli/src/commands/dblint.rs b/crates/pgt_cli/src/commands/dblint.rs new file mode 100644 index 000000000..ad8152601 --- /dev/null +++ b/crates/pgt_cli/src/commands/dblint.rs @@ -0,0 +1,18 @@ +use crate::cli_options::CliOptions; +use crate::reporter::Report; +use crate::{CliDiagnostic, CliSession, VcsIntegration}; +use pgt_configuration::PartialConfiguration; + +pub fn dblint( + mut session: CliSession, + cli_options: &CliOptions, + cli_configuration: Option, +) -> Result<(), CliDiagnostic> { + let configuration = session.prepare_with_config(cli_options, cli_configuration)?; + session.setup_workspace(configuration, VcsIntegration::Disabled)?; + + // TODO: Implement actual dblint logic here + let report = Report::new(vec![], std::time::Duration::new(0, 0), 0, None); + + session.report("dblint", cli_options, &report) +} diff --git a/crates/pgt_cli/src/commands/mod.rs b/crates/pgt_cli/src/commands/mod.rs index 043660be9..0e7ee9c22 100644 --- a/crates/pgt_cli/src/commands/mod.rs +++ b/crates/pgt_cli/src/commands/mod.rs @@ -1,23 +1,17 @@ use crate::changed::{get_changed_files, get_staged_files}; use crate::cli_options::{CliOptions, CliReporter, ColorsArg, cli_options}; -use crate::execute::Stdin; use crate::logging::LoggingKind; -use crate::{ - CliDiagnostic, CliSession, Execution, LoggingLevel, VERSION, execute_mode, setup_cli_subscriber, -}; +use crate::{CliDiagnostic, LoggingLevel, VERSION}; use bpaf::Bpaf; use pgt_configuration::{PartialConfiguration, partial_configuration}; -use pgt_console::{Console, ConsoleExt, markup}; -use pgt_fs::{ConfigName, FileSystem}; -use pgt_workspace::PartialConfigurationExt; -use pgt_workspace::configuration::{LoadedConfiguration, load_configuration}; -use pgt_workspace::workspace::{RegisterProjectFolderParams, UpdateSettingsParams}; -use pgt_workspace::{DynRef, Workspace, WorkspaceError}; +use pgt_fs::FileSystem; +use pgt_workspace::DynRef; use std::ffi::OsString; use std::path::PathBuf; pub(crate) mod check; pub(crate) mod clean; pub(crate) mod daemon; +pub(crate) mod dblint; pub(crate) mod init; pub(crate) mod version; @@ -286,145 +280,7 @@ impl PgtCommand { } } -/// Generic interface for executing commands. -/// -/// Consumers must implement the following methods: -/// -/// - [CommandRunner::merge_configuration] -/// - [CommandRunner::get_files_to_process] -/// - [CommandRunner::get_stdin_file_path] -/// - [CommandRunner::should_write] -/// - [CommandRunner::get_execution] -/// -/// Optional methods: -/// - [CommandRunner::check_incompatible_arguments] -pub(crate) trait CommandRunner: Sized { - const COMMAND_NAME: &'static str; - - /// The main command to use. - fn run(&mut self, session: CliSession, cli_options: &CliOptions) -> Result<(), CliDiagnostic> { - setup_cli_subscriber(cli_options.log_level, cli_options.log_kind); - let fs = &session.app.fs; - let console = &mut *session.app.console; - let workspace = &*session.app.workspace; - self.check_incompatible_arguments()?; - let (execution, paths) = self.configure_workspace(fs, console, workspace, cli_options)?; - execute_mode(execution, session, cli_options, paths) - } - - /// This function prepares the workspace with the following: - /// - Loading the configuration file. - /// - Configure the VCS integration - /// - Computes the paths to traverse/handle. This changes based on the VCS arguments that were passed. - /// - Register a project folder using the working directory. - /// - Updates the settings that belong to the project registered - fn configure_workspace( - &mut self, - fs: &DynRef<'_, dyn FileSystem>, - console: &mut dyn Console, - workspace: &dyn Workspace, - cli_options: &CliOptions, - ) -> Result<(Execution, Vec), CliDiagnostic> { - let loaded_configuration = - load_configuration(fs, cli_options.as_configuration_path_hint())?; - - // Check for deprecated config filename - if let Some(config_path) = &loaded_configuration.file_path { - if let Some(file_name) = config_path.file_name().and_then(|n| n.to_str()) { - if ConfigName::is_deprecated(file_name) { - console.log(markup! { - "Warning: ""You are using the deprecated config filename '""postgrestools.jsonc""'. \ - Please rename it to '""postgres-language-server.jsonc""'. \ - Support for the old filename will be removed in a future version.\n" - }); - } - } - } - - let configuration_path = loaded_configuration.directory_path.clone(); - let configuration = self.merge_configuration(loaded_configuration, fs, console)?; - let vcs_base_path = configuration_path.or(fs.working_directory()); - let (vcs_base_path, gitignore_matches) = - configuration.retrieve_gitignore_matches(fs, vcs_base_path.as_deref())?; - let paths = self.get_files_to_process(fs, &configuration)?; - workspace.register_project_folder(RegisterProjectFolderParams { - path: fs.working_directory(), - set_as_current_workspace: true, - })?; - - workspace.update_settings(UpdateSettingsParams { - workspace_directory: fs.working_directory(), - configuration, - vcs_base_path, - gitignore_matches, - })?; - - let execution = self.get_execution(cli_options, console, workspace)?; - Ok((execution, paths)) - } - - /// Computes [Stdin] if the CLI has the necessary information. - /// - /// ## Errors - /// - If the user didn't provide anything via `stdin` but the option `--stdin-file-path` is passed. - fn get_stdin(&self, console: &mut dyn Console) -> Result, CliDiagnostic> { - let stdin = if let Some(stdin_file_path) = self.get_stdin_file_path() { - let input_code = console.read(); - if let Some(input_code) = input_code { - let path = PathBuf::from(stdin_file_path); - Some((path, input_code).into()) - } else { - // we provided the argument without a piped stdin, we bail - return Err(CliDiagnostic::missing_argument("stdin", Self::COMMAND_NAME)); - } - } else { - None - }; - - Ok(stdin) - } - - // Below, the methods that consumers must implement. - - /// Implements this method if you need to merge CLI arguments to the loaded configuration. - /// - /// The CLI arguments take precedence over the option configured in the configuration file. - fn merge_configuration( - &mut self, - loaded_configuration: LoadedConfiguration, - fs: &DynRef<'_, dyn FileSystem>, - console: &mut dyn Console, - ) -> Result; - - /// It returns the paths that need to be handled/traversed. - fn get_files_to_process( - &self, - fs: &DynRef<'_, dyn FileSystem>, - configuration: &PartialConfiguration, - ) -> Result, CliDiagnostic>; - - /// It returns the file path to use in `stdin` mode. - fn get_stdin_file_path(&self) -> Option<&str>; - - /// Returns the [Execution] mode. - fn get_execution( - &self, - cli_options: &CliOptions, - console: &mut dyn Console, - workspace: &dyn Workspace, - ) -> Result; - - // Below, methods that consumers can implement - - /// Optional method that can be implemented to check if some CLI arguments aren't compatible. - /// - /// The method is called before loading the configuration from disk. - fn check_incompatible_arguments(&self) -> Result<(), CliDiagnostic> { - Ok(()) - } -} - -fn get_files_to_process_with_cli_options( +pub(crate) fn get_files_to_process_with_cli_options( since: Option<&str>, changed: bool, staged: bool, diff --git a/crates/pgt_cli/src/commands/version.rs b/crates/pgt_cli/src/commands/version.rs index 685684a31..0c9247295 100644 --- a/crates/pgt_cli/src/commands/version.rs +++ b/crates/pgt_cli/src/commands/version.rs @@ -5,22 +5,28 @@ use pgt_workspace::workspace::ServerInfo; use crate::{CliDiagnostic, CliSession, VERSION}; /// Handle of the `version` command. Prints a more in detail version. -pub(crate) fn full_version(session: CliSession) -> Result<(), CliDiagnostic> { - session.app.console.log(markup! { - "CLI: "{VERSION} - }); +pub(crate) fn version(mut session: CliSession) -> Result<(), CliDiagnostic> { + { + let console = session.console(); + console.log(markup! { + "CLI: "{VERSION} + }); + } + + let server_info = session.workspace().server_info().cloned(); + let console = session.console(); - match session.app.workspace.server_info() { + match server_info { None => { - session.app.console.log(markup! { + console.log(markup! { "Server: ""not connected" }); } Some(info) => { - session.app.console.log(markup! { + console.log(markup! { "Server: Name: "{info.name}" - Version: "{DisplayServerVersion(info)} + Version: "{DisplayServerVersion(&info)} }); } }; diff --git a/crates/pgt_cli/src/execute/config.rs b/crates/pgt_cli/src/execute/config.rs new file mode 100644 index 000000000..5ba3a6ef3 --- /dev/null +++ b/crates/pgt_cli/src/execute/config.rs @@ -0,0 +1,63 @@ +#[derive(Debug, Clone)] +pub struct ExecutionConfig { + pub mode: ExecutionMode, + pub max_diagnostics: u32, + pub allow_writes: bool, +} + +impl ExecutionConfig { + pub fn new(mode: ExecutionMode, max_diagnostics: u32) -> Self { + let allow_writes = mode.allows_writes(); + Self { + mode, + max_diagnostics, + allow_writes, + } + } + + pub fn max_diagnostics(&self) -> u32 { + self.max_diagnostics + } + + pub fn allows_writes(&self) -> bool { + self.allow_writes + } +} + +#[derive(Debug, Clone)] +pub enum ExecutionMode { + Check { vcs: VcsTargeting }, +} + +impl ExecutionMode { + pub fn allows_writes(&self) -> bool { + false + } + + pub fn vcs(&self) -> &VcsTargeting { + match self { + ExecutionMode::Check { vcs } => vcs, + } + } + + pub fn command_name(&self) -> &str { + match self { + ExecutionMode::Check { .. } => "check", + } + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub struct VcsTargeting { + pub staged: bool, + pub changed: bool, +} + +impl From<(bool, bool)> for VcsTargeting { + fn from(value: (bool, bool)) -> Self { + Self { + staged: value.0, + changed: value.1, + } + } +} diff --git a/crates/pgt_cli/src/execute/mod.rs b/crates/pgt_cli/src/execute/mod.rs index a8351fc98..b0485d362 100644 --- a/crates/pgt_cli/src/execute/mod.rs +++ b/crates/pgt_cli/src/execute/mod.rs @@ -1,303 +1,34 @@ +mod config; mod diagnostics; mod process_file; -mod std_in; -pub(crate) mod traverse; +mod stdin; +mod walk; -use crate::cli_options::{CliOptions, CliReporter}; -use crate::execute::traverse::{TraverseResult, traverse}; -use crate::reporter::github::{GithubReporter, GithubReporterVisitor}; -use crate::reporter::gitlab::{GitLabReporter, GitLabReporterVisitor}; -use crate::reporter::junit::{JunitReporter, JunitReporterVisitor}; -use crate::reporter::terminal::{ConsoleReporter, ConsoleReporterVisitor}; -use crate::{CliDiagnostic, CliSession, DiagnosticsPayload, Reporter}; -use pgt_diagnostics::{Category, category}; -use std::borrow::Borrow; +pub use config::{ExecutionConfig, ExecutionMode, VcsTargeting}; + +use crate::reporter::Report; +use crate::{CliDiagnostic, CliSession}; use std::ffi::OsString; -use std::fmt::{Display, Formatter}; use std::path::PathBuf; -use tracing::info; - -/// Useful information during the traversal of files and virtual content -#[derive(Debug, Clone)] -pub struct Execution { - /// How the information should be collected and reported - report_mode: ReportMode, - - /// The modality of execution of the traversal - traversal_mode: TraversalMode, - - /// The maximum number of diagnostics that can be printed in console - max_diagnostics: u32, -} - -impl Execution { - pub fn report_mode(&self) -> &ReportMode { - &self.report_mode - } -} - -/// A type that holds the information to execute the CLI via `stdin -#[derive(Debug, Clone)] -pub struct Stdin( - #[allow(unused)] - /// The virtual path to the file - PathBuf, - /// The content of the file - String, -); - -impl Stdin { - fn as_content(&self) -> &str { - self.1.as_str() - } -} - -impl From<(PathBuf, String)> for Stdin { - fn from((path, content): (PathBuf, String)) -> Self { - Self(path, content) - } -} - -#[derive(Debug, Clone)] -pub struct VcsTargeted { - pub staged: bool, - pub changed: bool, -} - -impl From<(bool, bool)> for VcsTargeted { - fn from((staged, changed): (bool, bool)) -> Self { - Self { staged, changed } - } -} - -#[derive(Debug, Clone)] -pub enum TraversalMode { - /// A dummy mode to be used when the CLI is not running any command - Dummy, - /// This mode is enabled when running the command `check` - Check { - /// An optional tuple. - /// 1. The virtual path to the file - /// 2. The content of the file - stdin: Option, - /// A flag to know vcs integrated options such as `--staged` or `--changed` are enabled - vcs_targeted: VcsTargeted, - }, -} - -impl Display for TraversalMode { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - match self { - TraversalMode::Dummy => write!(f, "dummy"), - TraversalMode::Check { .. } => write!(f, "check"), - } - } -} - -/// Tells to the execution of the traversal how the information should be reported -#[derive(Copy, Clone, Debug)] -pub enum ReportMode { - /// Reports information straight to the console, it's the default mode - Terminal, - /// Reports information for GitHub - GitHub, - /// JUnit output - /// Ref: https://github.com/testmoapp/junitxml?tab=readme-ov-file#basic-junit-xml-structure - Junit, - /// Reports information in the [GitLab Code Quality](https://docs.gitlab.com/ee/ci/testing/code_quality.html#implement-a-custom-tool) format. - GitLab, -} -impl Default for ReportMode { - fn default() -> Self { - Self::Terminal {} - } +pub fn run_files( + session: &mut CliSession, + config: &ExecutionConfig, + paths: Vec, +) -> Result { + walk::traverse(session, config, paths) } -impl From for ReportMode { - fn from(value: CliReporter) -> Self { - match value { - CliReporter::Default => Self::Terminal, - CliReporter::GitHub => Self::GitHub, - CliReporter::Junit => Self::Junit, - CliReporter::GitLab => Self::GitLab {}, - } - } +pub struct StdinPayload { + #[allow(dead_code)] + pub path: PathBuf, + pub content: String, } -impl Execution { - pub(crate) fn new(mode: TraversalMode) -> Self { - Self { - report_mode: ReportMode::default(), - traversal_mode: mode, - max_diagnostics: 20, - } - } - - /// It sets the reporting mode by reading the [CliOptions] - pub(crate) fn set_report(mut self, cli_options: &CliOptions) -> Self { - self.report_mode = cli_options.reporter.clone().into(); - self - } - - pub(crate) fn traversal_mode(&self) -> &TraversalMode { - &self.traversal_mode - } - - pub(crate) fn get_max_diagnostics(&self) -> u32 { - self.max_diagnostics - } - - pub(crate) fn as_diagnostic_category(&self) -> &'static Category { - match self.traversal_mode { - TraversalMode::Dummy => category!("dummy"), - TraversalMode::Check { .. } => category!("check"), - } - } - - /// Whether the traversal mode requires write access to files - pub(crate) const fn requires_write_access(&self) -> bool { - match self.traversal_mode { - TraversalMode::Dummy => false, - TraversalMode::Check { .. } => false, - } - } - - pub(crate) fn as_stdin_file(&self) -> Option<&Stdin> { - match &self.traversal_mode { - TraversalMode::Dummy => None, - TraversalMode::Check { stdin, .. } => stdin.as_ref(), - } - } - - pub(crate) fn is_vcs_targeted(&self) -> bool { - match &self.traversal_mode { - TraversalMode::Dummy => false, - TraversalMode::Check { vcs_targeted, .. } => { - vcs_targeted.staged || vcs_targeted.changed - } - } - } - - pub(crate) const fn is_check_apply(&self) -> bool { - false - } - - #[allow(unused)] - /// Returns [true] if the user used the `--write`/`--fix` option - pub(crate) fn is_write(&self) -> bool { - match self.traversal_mode { - TraversalMode::Dummy => false, - TraversalMode::Check { .. } => false, - } - } -} - -/// Based on the [mode](TraversalMode), the function might launch a traversal of the file system -/// or handles the stdin file. -pub fn execute_mode( - mut execution: Execution, - mut session: CliSession, - cli_options: &CliOptions, - paths: Vec, +pub fn run_stdin( + session: &mut CliSession, + config: &ExecutionConfig, + payload: StdinPayload, ) -> Result<(), CliDiagnostic> { - // If a custom reporter was provided, let's lift the limit so users can see all of them - execution.max_diagnostics = if cli_options.reporter.is_default() { - cli_options.max_diagnostics.into() - } else { - info!( - "Removing the limit of --max-diagnostics, because of a reporter different from the default one: {}", - cli_options.reporter - ); - u32::MAX - }; - - // don't do any traversal if there's some content coming from stdin - if let Some(stdin) = execution.as_stdin_file() { - std_in::run(session, stdin.as_content()) - } else { - let TraverseResult { - summary, - evaluated_paths, - diagnostics, - } = traverse(&execution, &mut session, cli_options, paths)?; - let console = session.app.console; - let errors = summary.errors; - let skipped = summary.skipped; - let processed = summary.changed + summary.unchanged; - let should_exit_on_warnings = summary.warnings > 0 && cli_options.error_on_warnings; - - match execution.report_mode { - ReportMode::Terminal => { - let reporter = ConsoleReporter { - summary, - diagnostics_payload: DiagnosticsPayload { - verbose: cli_options.verbose, - diagnostic_level: cli_options.diagnostic_level, - diagnostics, - }, - execution: execution.clone(), - evaluated_paths, - }; - reporter.write(&mut ConsoleReporterVisitor(console))?; - } - ReportMode::GitHub => { - let reporter = GithubReporter { - diagnostics_payload: DiagnosticsPayload { - verbose: cli_options.verbose, - diagnostic_level: cli_options.diagnostic_level, - diagnostics, - }, - execution: execution.clone(), - }; - reporter.write(&mut GithubReporterVisitor(console))?; - } - ReportMode::GitLab => { - let reporter = GitLabReporter { - diagnostics: DiagnosticsPayload { - verbose: cli_options.verbose, - diagnostic_level: cli_options.diagnostic_level, - diagnostics, - }, - execution: execution.clone(), - }; - reporter.write(&mut GitLabReporterVisitor::new( - console, - session.app.fs.borrow().working_directory(), - ))?; - } - ReportMode::Junit => { - let reporter = JunitReporter { - summary, - diagnostics_payload: DiagnosticsPayload { - verbose: cli_options.verbose, - diagnostic_level: cli_options.diagnostic_level, - diagnostics, - }, - execution: execution.clone(), - }; - reporter.write(&mut JunitReporterVisitor::new(console))?; - } - } - - // Processing emitted error diagnostics, exit with a non-zero code - if processed.saturating_sub(skipped) == 0 && !cli_options.no_errors_on_unmatched { - Err(CliDiagnostic::no_files_processed()) - } else if errors > 0 || should_exit_on_warnings { - let category = execution.as_diagnostic_category(); - if should_exit_on_warnings { - if execution.is_check_apply() { - Err(CliDiagnostic::apply_warnings(category)) - } else { - Err(CliDiagnostic::check_warnings(category)) - } - } else if execution.is_check_apply() { - Err(CliDiagnostic::apply_error(category)) - } else { - Err(CliDiagnostic::check_error(category)) - } - } else { - Ok(()) - } - } + stdin::process(session, config, payload) } diff --git a/crates/pgt_cli/src/execute/process_file.rs b/crates/pgt_cli/src/execute/process_file.rs index 421f9bb31..b35e85e66 100644 --- a/crates/pgt_cli/src/execute/process_file.rs +++ b/crates/pgt_cli/src/execute/process_file.rs @@ -1,8 +1,8 @@ mod check; pub(crate) mod workspace_file; -use crate::execute::TraversalMode; -use crate::execute::traverse::TraversalOptions; +use crate::execute::config::ExecutionMode; +use crate::execute::walk::TraversalOptions; use check::check_file; use pgt_diagnostics::Error; use pgt_fs::PgTPath; @@ -107,11 +107,8 @@ pub(crate) fn process_file(ctx: &TraversalOptions, pgt_path: &PgTPath) -> FileRe tracing::trace_span!("process_file", path = ?pgt_path).in_scope(move || { let shared_context = &SharedTraversalOptions::new(ctx); - match ctx.execution.traversal_mode { - TraversalMode::Dummy => { - unreachable!("The dummy mode should not be called for this file") - } - TraversalMode::Check { .. } => check_file(shared_context, pgt_path), + match ctx.config.mode { + ExecutionMode::Check { .. } => check_file(shared_context, pgt_path), } }) } diff --git a/crates/pgt_cli/src/execute/process_file/workspace_file.rs b/crates/pgt_cli/src/execute/process_file/workspace_file.rs index 9f78c7cf1..5236ff40e 100644 --- a/crates/pgt_cli/src/execute/process_file/workspace_file.rs +++ b/crates/pgt_cli/src/execute/process_file/workspace_file.rs @@ -10,7 +10,7 @@ use std::path::{Path, PathBuf}; pub(crate) struct WorkspaceFile<'ctx, 'app> { guard: FileGuard<'app, dyn Workspace + 'ctx>, #[allow(dead_code)] - file: Box, + file: Option>, // only present when backed by a real fs entry pub(crate) path: PathBuf, } @@ -24,7 +24,7 @@ impl<'ctx, 'app> WorkspaceFile<'ctx, 'app> { let pgt_path = PgTPath::new(path); let open_options = OpenOptions::default() .read(true) - .write(ctx.execution.requires_write_access()); + .write(ctx.config.allows_writes()); let mut file = ctx .fs .open_with_options(path, open_options) @@ -45,7 +45,7 @@ impl<'ctx, 'app> WorkspaceFile<'ctx, 'app> { .with_file_path_and_code(path.display().to_string(), category!("internalError/fs"))?; Ok(Self { - file, + file: Some(file), guard, path: PathBuf::from(path), }) diff --git a/crates/pgt_cli/src/execute/std_in.rs b/crates/pgt_cli/src/execute/std_in.rs deleted file mode 100644 index f9346f6a1..000000000 --- a/crates/pgt_cli/src/execute/std_in.rs +++ /dev/null @@ -1,11 +0,0 @@ -//! In here, there are the operations that run via standard input -//! -use crate::{CliDiagnostic, CliSession}; -use pgt_console::{ConsoleExt, markup}; - -pub(crate) fn run(session: CliSession, content: &str) -> Result<(), CliDiagnostic> { - let console = &mut *session.app.console; - - console.append(markup! {{content}}); - Ok(()) -} diff --git a/crates/pgt_cli/src/execute/stdin.rs b/crates/pgt_cli/src/execute/stdin.rs new file mode 100644 index 000000000..095a607ca --- /dev/null +++ b/crates/pgt_cli/src/execute/stdin.rs @@ -0,0 +1,13 @@ +use crate::execute::StdinPayload; +use crate::execute::config::ExecutionConfig; +use crate::{CliDiagnostic, CliSession}; +use pgt_console::{ConsoleExt, markup}; + +pub(crate) fn process( + session: &mut CliSession, + _config: &ExecutionConfig, + payload: StdinPayload, +) -> Result<(), CliDiagnostic> { + session.console().append(markup! {{payload.content}}); + Ok(()) +} diff --git a/crates/pgt_cli/src/execute/traverse.rs b/crates/pgt_cli/src/execute/walk.rs similarity index 66% rename from crates/pgt_cli/src/execute/traverse.rs rename to crates/pgt_cli/src/execute/walk.rs index 5673810c9..bb156e9a1 100644 --- a/crates/pgt_cli/src/execute/traverse.rs +++ b/crates/pgt_cli/src/execute/walk.rs @@ -1,12 +1,10 @@ +use super::config::ExecutionConfig; use super::process_file::{FileStatus, Message, process_file}; -use super::{Execution, TraversalMode}; -use crate::cli_options::CliOptions; use crate::execute::diagnostics::PanicDiagnostic; -use crate::reporter::TraversalSummary; +use crate::reporter::{Report, TraversalData}; use crate::{CliDiagnostic, CliSession}; use crossbeam::channel::{Receiver, Sender, unbounded}; -use pgt_diagnostics::DiagnosticTags; -use pgt_diagnostics::{DiagnosticExt, Error, Resource, Severity}; +use pgt_diagnostics::{DiagnosticExt, Error, Resource}; use pgt_fs::{FileSystem, PathInterner, PgTPath}; use pgt_fs::{TraversalContext, TraversalScope}; use pgt_workspace::dome::Dome; @@ -29,39 +27,17 @@ use std::{ time::{Duration, Instant}, }; -pub(crate) struct TraverseResult { - pub(crate) summary: TraversalSummary, - pub(crate) evaluated_paths: BTreeSet, - pub(crate) diagnostics: Vec, -} - pub(crate) fn traverse( - execution: &Execution, session: &mut CliSession, - cli_options: &CliOptions, + config: &ExecutionConfig, mut inputs: Vec, -) -> Result { +) -> Result { init_thread_pool(); - if inputs.is_empty() { - match &execution.traversal_mode { - TraversalMode::Dummy => { - // If `--staged` or `--changed` is specified, it's acceptable for them to be empty, so ignore it. - if !execution.is_vcs_targeted() { - match current_dir() { - Ok(current_dir) => inputs.push(current_dir.into_os_string()), - Err(err) => return Err(CliDiagnostic::io_error(err)), - } - } - } - _ => { - if execution.as_stdin_file().is_none() && !cli_options.no_errors_on_unmatched { - return Err(CliDiagnostic::missing_argument( - "", - format!("{}", execution.traversal_mode), - )); - } - } + if inputs.is_empty() && !config.mode.vcs().changed && !config.mode.vcs().staged { + match current_dir() { + Ok(current_dir) => inputs.push(current_dir.into_os_string()), + Err(err) => return Err(CliDiagnostic::io_error(err)), } } @@ -73,16 +49,13 @@ pub(crate) fn traverse( let matches = AtomicUsize::new(0); let skipped = AtomicUsize::new(0); - let fs = &*session.app.fs; - let workspace = &*session.app.workspace; + let fs = &**session.fs(); + let workspace = session.workspace(); - let max_diagnostics = execution.get_max_diagnostics(); + let max_diagnostics = config.max_diagnostics(); let remaining_diagnostics = AtomicU32::new(max_diagnostics); - let printer = DiagnosticsPrinter::new(execution) - .with_verbose(cli_options.verbose) - .with_diagnostic_level(cli_options.diagnostic_level) - .with_max_diagnostics(max_diagnostics); + let printer = DiagnosticsPrinter::new(config).with_max_diagnostics(max_diagnostics); let (duration, evaluated_paths, diagnostics) = thread::scope(|s| { let handler = thread::Builder::new() @@ -98,7 +71,7 @@ pub(crate) fn traverse( &TraversalOptions { fs, workspace, - execution, + config, interner, matches: &matches, changed: &changed, @@ -115,8 +88,6 @@ pub(crate) fn traverse( (elapsed, evaluated_paths, diagnostics) }); - let errors = printer.errors(); - let warnings = printer.warnings(); let changed = changed.load(Ordering::Relaxed); let unchanged = unchanged.load(Ordering::Relaxed); let matches = matches.load(Ordering::Relaxed); @@ -124,21 +95,23 @@ pub(crate) fn traverse( let suggested_fixes_skipped = printer.skipped_fixes(); let diagnostics_not_printed = printer.not_printed_diagnostics(); - Ok(TraverseResult { - summary: TraversalSummary { - changed, - unchanged, - duration, - errors, - matches, - warnings, - skipped, - suggested_fixes_skipped, - diagnostics_not_printed, - }, + let traversal = TraversalData { evaluated_paths, + changed, + unchanged, + matches, + skipped, + suggested_fixes_skipped, + diagnostics_not_printed, + workspace_root: session.fs().working_directory(), + }; + + Ok(Report::new( diagnostics, - }) + duration, + diagnostics_not_printed, + Some(traversal), + )) } /// This function will setup the global Rayon thread pool the first time it's called @@ -187,69 +160,31 @@ fn traverse_inputs( // struct DiagnosticsReporter<'ctx> {} struct DiagnosticsPrinter<'ctx> { - /// Execution of the traversal - #[allow(dead_code)] - execution: &'ctx Execution, - /// The maximum number of diagnostics the console thread is allowed to print + _config: &'ctx ExecutionConfig, max_diagnostics: u32, - /// The approximate number of diagnostics the console will print before - /// folding the rest into the "skipped diagnostics" counter remaining_diagnostics: AtomicU32, - /// Mutable reference to a boolean flag tracking whether the console thread - /// printed any error-level message - errors: AtomicU32, - /// Mutable reference to a boolean flag tracking whether the console thread - /// printed any warnings-level message - warnings: AtomicU32, - /// Whether the console thread should print diagnostics in verbose mode - verbose: bool, - /// The diagnostic level the console thread should print - diagnostic_level: Severity, - not_printed_diagnostics: AtomicU32, printed_diagnostics: AtomicU32, total_skipped_suggested_fixes: AtomicU32, } impl<'ctx> DiagnosticsPrinter<'ctx> { - fn new(execution: &'ctx Execution) -> Self { + fn new(config: &'ctx ExecutionConfig) -> Self { Self { - errors: AtomicU32::new(0), - warnings: AtomicU32::new(0), - remaining_diagnostics: AtomicU32::new(0), - execution, - diagnostic_level: Severity::Hint, - verbose: false, + _config: config, max_diagnostics: 20, + remaining_diagnostics: AtomicU32::new(0), not_printed_diagnostics: AtomicU32::new(0), printed_diagnostics: AtomicU32::new(0), total_skipped_suggested_fixes: AtomicU32::new(0), } } - fn with_verbose(mut self, verbose: bool) -> Self { - self.verbose = verbose; - self - } - fn with_max_diagnostics(mut self, value: u32) -> Self { self.max_diagnostics = value; self } - fn with_diagnostic_level(mut self, value: Severity) -> Self { - self.diagnostic_level = value; - self - } - - fn errors(&self) -> u32 { - self.errors.load(Ordering::Relaxed) - } - - fn warnings(&self) -> u32 { - self.warnings.load(Ordering::Relaxed) - } - fn not_printed_diagnostics(&self) -> u32 { self.not_printed_diagnostics.load(Ordering::Relaxed) } @@ -258,23 +193,8 @@ impl<'ctx> DiagnosticsPrinter<'ctx> { self.total_skipped_suggested_fixes.load(Ordering::Relaxed) } - /// Checks if the diagnostic we received from the thread should be considered or not. Logic: - /// - it should not be considered if its severity level is lower than the one provided via CLI; - /// - it should not be considered if it's a verbose diagnostic and the CLI **didn't** request a `--verbose` option. - fn should_skip_diagnostic(&self, severity: Severity, diagnostic_tags: DiagnosticTags) -> bool { - if severity < self.diagnostic_level { - return true; - } - - if diagnostic_tags.is_verbose() && !self.verbose { - return true; - } - - false - } - /// Count the diagnostic, and then returns a boolean that tells if it should be printed - fn should_print(&self) -> bool { + fn should_store(&self) -> bool { let printed_diagnostics = self.printed_diagnostics.load(Ordering::Relaxed); let should_print = printed_diagnostics < self.max_diagnostics; if should_print { @@ -292,8 +212,7 @@ impl<'ctx> DiagnosticsPrinter<'ctx> { fn run(&self, receiver: Receiver, interner: Receiver) -> Vec { let mut paths: FxHashSet = FxHashSet::default(); - - let mut diagnostics_to_print = vec![]; + let mut diagnostics = vec![]; while let Ok(msg) = receiver.recv() { match msg { @@ -303,24 +222,9 @@ impl<'ctx> DiagnosticsPrinter<'ctx> { self.total_skipped_suggested_fixes .fetch_add(skipped_suggested_fixes, Ordering::Relaxed); } - - Message::Failure => { - self.errors.fetch_add(1, Ordering::Relaxed); - } - + Message::Failure => {} Message::Error(mut err) => { - let location = err.location(); - if self.should_skip_diagnostic(err.severity(), err.tags()) { - continue; - } - if err.severity() == Severity::Warning { - // *warnings += 1; - self.warnings.fetch_add(1, Ordering::Relaxed); - // self.warnings.set(self.warnings.get() + 1) - } - if let Some(Resource::File(file_path)) = location.resource.as_ref() { - // Retrieves the file name from the file ID cache, if it's a miss - // flush entries from the interner channel until it's found + if let Some(Resource::File(file_path)) = err.location().resource.as_ref() { let file_name = match paths.get(*file_path) { Some(path) => Some(path), None => loop { @@ -331,9 +235,6 @@ impl<'ctx> DiagnosticsPrinter<'ctx> { break paths.get(&path.display().to_string()); } } - // In case the channel disconnected without sending - // the path we need, print the error without a file - // name (normally this should never happen) Err(_) => break None, } }, @@ -344,47 +245,30 @@ impl<'ctx> DiagnosticsPrinter<'ctx> { } } - let should_print = self.should_print(); - - if should_print { - diagnostics_to_print.push(err); + if self.should_store() { + diagnostics.push(err); } } - Message::Diagnostics { name, content, - diagnostics, + diagnostics: diag_list, skipped_diagnostics, } => { self.not_printed_diagnostics .fetch_add(skipped_diagnostics, Ordering::Relaxed); - // is CI mode we want to print all the diagnostics - for diag in diagnostics { - let severity = diag.severity(); - if self.should_skip_diagnostic(severity, diag.tags()) { - continue; - } - if severity == Severity::Error { - self.errors.fetch_add(1, Ordering::Relaxed); - } - if severity == Severity::Warning { - self.warnings.fetch_add(1, Ordering::Relaxed); - } - - let should_print = self.should_print(); - - if should_print { + for diag in diag_list { + if self.should_store() { let diag = diag.with_file_path(&name).with_file_source_code(&content); - diagnostics_to_print.push(diag) + diagnostics.push(diag); } } } } } - diagnostics_to_print + diagnostics } } @@ -395,7 +279,7 @@ pub(crate) struct TraversalOptions<'ctx, 'app> { /// Instance of [Workspace] used by this instance of the CLI pub(crate) workspace: &'ctx dyn Workspace, /// Determines how the files should be processed - pub(crate) execution: &'ctx Execution, + pub(crate) config: &'ctx ExecutionConfig, /// File paths interner cache used by the filesystem traversal interner: PathInterner, /// Shared atomic counter storing the number of changed files @@ -483,10 +367,7 @@ impl TraversalContext for TraversalOptions<'_, '_> { return false; } - match self.execution.traversal_mode() { - TraversalMode::Dummy => true, - TraversalMode::Check { .. } => true, - } + true } fn handle_path(&self, path: PgTPath) { diff --git a/crates/pgt_cli/src/lib.rs b/crates/pgt_cli/src/lib.rs index 1e1199bde..c350293be 100644 --- a/crates/pgt_cli/src/lib.rs +++ b/crates/pgt_cli/src/lib.rs @@ -4,11 +4,12 @@ //! to parse commands and arguments, redirect the execution of the commands and //! execute the traversal of directory and files, based on the command that was passed. +use biome_deserialize::Merge; use cli_options::CliOptions; -use commands::CommandRunner; -use commands::check::CheckCommandPayload; -use pgt_console::{ColorMode, Console}; -use pgt_fs::OsFileSystem; +use commands::check::{self, CheckArgs}; +use pgt_configuration::PartialConfiguration; +use pgt_console::{ColorMode, Console, ConsoleExt, markup}; +use pgt_fs::{ConfigName, FileSystem, OsFileSystem}; use pgt_workspace::{App, DynRef, Workspace, WorkspaceRef}; mod changed; @@ -21,14 +22,16 @@ mod metrics; mod panic; mod reporter; mod service; +mod workspace; use crate::cli_options::ColorsArg; pub use crate::commands::{PgtCommand, pgt_command}; pub use crate::logging::{LoggingLevel, setup_cli_subscriber}; +use crate::reporter::Report; pub use diagnostics::CliDiagnostic; -pub use execute::{Execution, TraversalMode, VcsTargeted, execute_mode}; +pub use execute::{ExecutionConfig, ExecutionMode, VcsTargeting}; pub use panic::setup_panic_handler; -pub use reporter::{DiagnosticsPayload, Reporter, ReporterVisitor, TraversalSummary}; +pub use reporter::{ReportConfig, Reporter, TraversalData}; pub use service::{SocketTransport, open_transport}; pub(crate) use pgt_env::VERSION; @@ -61,7 +64,11 @@ impl<'app> CliSession<'app> { } let result = match command { - PgtCommand::Version(_) => commands::version::full_version(self), + PgtCommand::Version(_) => commands::version::version(self), + PgtCommand::Dblint { + cli_options, + configuration, + } => commands::dblint::dblint(self, &cli_options, configuration), PgtCommand::Check { cli_options, configuration, @@ -70,16 +77,18 @@ impl<'app> CliSession<'app> { staged, changed, since, - } => run_command( + } => check::check( self, &cli_options, - CheckCommandPayload { + CheckArgs { configuration, paths, stdin_file_path, staged, changed, since, + apply: false, + apply_unsafe: false, }, ), PgtCommand::Clean => commands::clean::clean(self), @@ -120,6 +129,72 @@ impl<'app> CliSession<'app> { result } + + pub fn fs(&self) -> &DynRef<'app, dyn FileSystem> { + &self.app.fs + } + + pub fn console(&mut self) -> &mut (dyn Console + 'app) { + &mut *self.app.console + } + + pub fn workspace(&self) -> &(dyn Workspace + 'app) { + &*self.app.workspace + } + + pub fn prepare_with_config( + &mut self, + cli_options: &CliOptions, + cli_configuration: Option, + ) -> Result { + setup_cli_subscriber(cli_options.log_level, cli_options.log_kind); + + let fs = self.fs(); + let loaded_configuration = + workspace::load_config(fs, cli_options.as_configuration_path_hint())?; + + if let Some(config_path) = &loaded_configuration.file_path { + if let Some(file_name) = config_path.file_name().and_then(|name| name.to_str()) { + if ConfigName::is_deprecated(file_name) { + self.console().log(markup! { + "Warning: " + "Deprecated config filename detected. Use 'postgres-language-server.jsonc'.\n" + }); + } + } + } + + let mut configuration = loaded_configuration.configuration; + if let Some(cli_config) = cli_configuration { + configuration.merge_with(cli_config); + } + + Ok(configuration) + } + + pub fn setup_workspace( + &mut self, + configuration: PartialConfiguration, + vcs: VcsIntegration, + ) -> Result<(), CliDiagnostic> { + workspace::setup_workspace(self.workspace(), self.fs(), configuration, vcs) + } + + pub fn report( + &mut self, + command_name: &str, + cli_options: &CliOptions, + payload: &Report, + ) -> Result<(), CliDiagnostic> { + let mut reporter = Reporter::from_cli_options(cli_options); + reporter.report(self.console(), command_name, payload) + } +} + +/// Controls whether workspace setup should include VCS integration details. +pub enum VcsIntegration { + Enabled, + Disabled, } pub fn to_color_mode(color: Option<&ColorsArg>) -> ColorMode { @@ -129,12 +204,3 @@ pub fn to_color_mode(color: Option<&ColorsArg>) -> ColorMode { None => ColorMode::Auto, } } - -pub(crate) fn run_command( - session: CliSession, - cli_options: &CliOptions, - mut command: impl CommandRunner, -) -> Result<(), CliDiagnostic> { - let command = &mut command; - command.run(session, cli_options) -} diff --git a/crates/pgt_cli/src/reporter/github.rs b/crates/pgt_cli/src/reporter/github.rs index 1faa97414..0d11d4d7f 100644 --- a/crates/pgt_cli/src/reporter/github.rs +++ b/crates/pgt_cli/src/reporter/github.rs @@ -1,42 +1,29 @@ -use crate::{DiagnosticsPayload, Execution, Reporter, ReporterVisitor, TraversalSummary}; +use crate::diagnostics::CliDiagnostic; +use crate::reporter::{Report, ReportConfig, ReportWriter}; use pgt_console::{Console, ConsoleExt, markup}; use pgt_diagnostics::PrintGitHubDiagnostic; -use std::io; -pub(crate) struct GithubReporter { - pub(crate) diagnostics_payload: DiagnosticsPayload, - pub(crate) execution: Execution, -} - -impl Reporter for GithubReporter { - fn write(self, visitor: &mut dyn ReporterVisitor) -> io::Result<()> { - visitor.report_diagnostics(&self.execution, self.diagnostics_payload)?; - Ok(()) - } -} -pub(crate) struct GithubReporterVisitor<'a>(pub(crate) &'a mut dyn Console); +pub(crate) struct GithubReportWriter; -impl ReporterVisitor for GithubReporterVisitor<'_> { - fn report_summary( +impl ReportWriter for GithubReportWriter { + fn write( &mut self, - _execution: &Execution, - _summary: TraversalSummary, - ) -> io::Result<()> { - Ok(()) - } + console: &mut dyn Console, + _command_name: &str, + report: &Report, + config: &ReportConfig, + ) -> Result<(), CliDiagnostic> { + for diagnostic in &report.diagnostics { + if diagnostic.severity() < config.diagnostic_level { + continue; + } - fn report_diagnostics( - &mut self, - _execution: &Execution, - diagnostics_payload: DiagnosticsPayload, - ) -> io::Result<()> { - for diagnostic in &diagnostics_payload.diagnostics { - if diagnostic.severity() >= diagnostics_payload.diagnostic_level { - if diagnostic.tags().is_verbose() && diagnostics_payload.verbose { - self.0.log(markup! {{PrintGitHubDiagnostic(diagnostic)}}); - } else if !diagnostics_payload.verbose { - self.0.log(markup! {{PrintGitHubDiagnostic(diagnostic)}}); + if diagnostic.tags().is_verbose() { + if config.verbose { + console.log(markup! {{PrintGitHubDiagnostic(diagnostic)}}); } + } else if !config.verbose { + console.log(markup! {{PrintGitHubDiagnostic(diagnostic)}}); } } diff --git a/crates/pgt_cli/src/reporter/gitlab.rs b/crates/pgt_cli/src/reporter/gitlab.rs index fc35a8e0e..1d89db492 100644 --- a/crates/pgt_cli/src/reporter/gitlab.rs +++ b/crates/pgt_cli/src/reporter/gitlab.rs @@ -1,4 +1,5 @@ -use crate::{DiagnosticsPayload, Execution, Reporter, ReporterVisitor, TraversalSummary}; +use crate::diagnostics::CliDiagnostic; +use crate::reporter::{Report, ReportConfig, ReportWriter}; use path_absolutize::Absolutize; use pgt_console::fmt::{Display, Formatter}; use pgt_console::{Console, ConsoleExt, markup}; @@ -12,29 +13,37 @@ use std::{ path::{Path, PathBuf}, }; -pub struct GitLabReporter { - pub(crate) execution: Execution, - pub(crate) diagnostics: DiagnosticsPayload, -} +pub(crate) struct GitLabReportWriter; + +impl ReportWriter for GitLabReportWriter { + fn write( + &mut self, + console: &mut dyn Console, + _command_name: &str, + report: &Report, + config: &ReportConfig, + ) -> Result<(), CliDiagnostic> { + let repository_root = report + .traversal + .as_ref() + .and_then(|traversal| traversal.workspace_root.clone()); -impl Reporter for GitLabReporter { - fn write(self, visitor: &mut dyn ReporterVisitor) -> std::io::Result<()> { - visitor.report_diagnostics(&self.execution, self.diagnostics)?; + let hasher = RwLock::default(); + let diagnostics = GitLabDiagnostics { + report, + config, + hasher: &hasher, + repository_root: repository_root.as_deref(), + }; + console.log(markup!({ diagnostics })); Ok(()) } } -pub(crate) struct GitLabReporterVisitor<'a> { - console: &'a mut dyn Console, - repository_root: Option, -} - #[derive(Default)] struct GitLabHasher(HashSet); impl GitLabHasher { - /// Enforces uniqueness of generated fingerprints in the context of a - /// single report. fn rehash_until_unique(&mut self, fingerprint: u64) -> u64 { let mut current = fingerprint; while self.0.contains(¤t) { @@ -48,45 +57,20 @@ impl GitLabHasher { } } -impl<'a> GitLabReporterVisitor<'a> { - pub fn new(console: &'a mut dyn Console, repository_root: Option) -> Self { - Self { - console, - repository_root, - } - } +struct GitLabDiagnostics<'a> { + report: &'a Report, + config: &'a ReportConfig, + hasher: &'a RwLock, + repository_root: Option<&'a Path>, } -impl ReporterVisitor for GitLabReporterVisitor<'_> { - fn report_summary(&mut self, _: &Execution, _: TraversalSummary) -> std::io::Result<()> { - Ok(()) - } - - fn report_diagnostics( - &mut self, - _execution: &Execution, - payload: DiagnosticsPayload, - ) -> std::io::Result<()> { - let hasher = RwLock::default(); - let diagnostics = GitLabDiagnostics(payload, &hasher, self.repository_root.as_deref()); - self.console.log(markup!({ diagnostics })); - Ok(()) - } -} - -struct GitLabDiagnostics<'a>( - DiagnosticsPayload, - &'a RwLock, - Option<&'a Path>, -); - -impl GitLabDiagnostics<'_> { +impl<'a> GitLabDiagnostics<'a> { fn attempt_to_relativize(&self, subject: &str) -> Option { let Ok(resolved) = Path::new(subject).absolutize() else { return None; }; - let Ok(relativized) = resolved.strip_prefix(self.2?) else { + let Ok(relativized) = resolved.strip_prefix(self.repository_root?) else { return None; }; @@ -118,14 +102,14 @@ impl GitLabDiagnostics<'_> { impl Display for GitLabDiagnostics<'_> { fn fmt(&self, fmt: &mut Formatter) -> std::io::Result<()> { - let mut hasher = self.1.write().unwrap(); + let mut hasher = self.hasher.write().unwrap(); let gitlab_diagnostics: Vec<_> = self - .0 + .report .diagnostics .iter() - .filter(|d| d.severity() >= self.0.diagnostic_level) + .filter(|d| d.severity() >= self.config.diagnostic_level) .filter(|d| { - if self.0.verbose { + if self.config.verbose { d.tags().is_verbose() } else { true @@ -155,19 +139,12 @@ impl Display for GitLabDiagnostics<'_> { } } -/// An entry in the GitLab Code Quality report. -/// See https://docs.gitlab.com/ee/ci/testing/code_quality.html#implement-a-custom-tool #[derive(Serialize)] pub struct GitLabDiagnostic<'a> { - /// A description of the code quality violation. description: String, - /// A unique name representing the static analysis check that emitted this issue. check_name: &'a str, - /// A unique fingerprint to identify the code quality violation. For example, an MD5 hash. fingerprint: String, - /// A severity string (can be info, minor, major, critical, or blocker). severity: &'a str, - /// The location where the code quality violation occurred. location: Location, } @@ -200,42 +177,35 @@ impl<'a> GitLabDiagnostic<'a> { }, description, check_name, - // A u64 does not fit into a JSON number, so we serialize this as a - // string fingerprint: fingerprint.to_string(), location: Location { path, - lines: Lines { begin }, + lines: GitLabLines { begin }, }, }) } } #[derive(Serialize)] -struct Location { - /// The relative path to the file containing the code quality violation. +pub struct Location { path: String, - lines: Lines, + lines: GitLabLines, } #[derive(Serialize)] -struct Lines { - /// The line on which the code quality violation occurred. +pub struct GitLabLines { begin: usize, } -#[derive(Hash)] -struct Fingerprint<'a> { - // Including the source code in our hash leads to more stable - // fingerprints. If you instead rely on e.g. the line number and change - // the first line of a file, all of its fingerprint would change. +#[derive(Hash, Serialize)] +pub struct Fingerprint<'a> { code: &'a str, check_name: &'a str, path: &'a str, } fn calculate_hash(t: &T) -> u64 { - let mut s = DefaultHasher::new(); - t.hash(&mut s); - s.finish() + let mut hasher = DefaultHasher::new(); + t.hash(&mut hasher); + hasher.finish() } diff --git a/crates/pgt_cli/src/reporter/junit.rs b/crates/pgt_cli/src/reporter/junit.rs index 670bf8d40..e5d1c5d67 100644 --- a/crates/pgt_cli/src/reporter/junit.rs +++ b/crates/pgt_cli/src/reporter/junit.rs @@ -1,119 +1,105 @@ -use crate::{DiagnosticsPayload, Execution, Reporter, ReporterVisitor, TraversalSummary}; +use crate::diagnostics::CliDiagnostic; +use crate::reporter::{Report, ReportConfig, ReportWriter}; use pgt_console::{Console, ConsoleExt, markup}; use pgt_diagnostics::display::SourceFile; use pgt_diagnostics::{Error, Resource}; -use quick_junit::{NonSuccessKind, Report, TestCase, TestCaseStatus, TestSuite}; +use quick_junit::{NonSuccessKind, Report as JunitReport, TestCase, TestCaseStatus, TestSuite}; use std::fmt::{Display, Formatter}; -use std::io; -pub(crate) struct JunitReporter { - pub(crate) diagnostics_payload: DiagnosticsPayload, - pub(crate) execution: Execution, - pub(crate) summary: TraversalSummary, -} +pub(crate) struct JunitReportWriter; -impl Reporter for JunitReporter { - fn write(self, visitor: &mut dyn ReporterVisitor) -> io::Result<()> { - visitor.report_summary(&self.execution, self.summary)?; - visitor.report_diagnostics(&self.execution, self.diagnostics_payload)?; +impl ReportWriter for JunitReportWriter { + fn write( + &mut self, + console: &mut dyn Console, + command_name: &str, + report: &Report, + config: &ReportConfig, + ) -> Result<(), CliDiagnostic> { + let mut junit = JunitReport::new(command_name); + junit.time = Some(report.duration); + junit.errors = report.errors as usize; + append_diagnostics(&mut junit, config, &report.diagnostics)?; + + console.log(markup! {{junit.to_string().unwrap()}}); Ok(()) } } -struct JunitDiagnostic<'a> { - diagnostic: &'a Error, -} - -impl Display for JunitDiagnostic<'_> { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - self.diagnostic.description(f) +fn append_diagnostics( + report: &mut JunitReport, + config: &ReportConfig, + diagnostics: &[Error], +) -> Result<(), CliDiagnostic> { + for diagnostic in diagnostics.iter().filter(|diag| should_emit(config, diag)) { + let mut status = TestCaseStatus::non_success(NonSuccessKind::Failure); + let message = format!("{}", JunitDiagnostic { diagnostic }); + status.set_message(message.clone()); + + let location = diagnostic.location(); + + if let (Some(span), Some(source_code), Some(resource)) = + (location.span, location.source_code, location.resource) + { + let source = SourceFile::new(source_code); + let start = source + .location(span.start()) + .map_err(CliDiagnostic::io_error)?; + + status.set_description(format!( + "line {row:?}, col {col:?}, {body}", + row = start.line_number.to_zero_indexed(), + col = start.column_number.to_zero_indexed(), + body = message + )); + let mut case = TestCase::new( + format!( + "org.pgls.{}", + diagnostic + .category() + .map(|c| c.name()) + .unwrap_or_default() + .replace('/', ".") + ), + status, + ); + + if let Resource::File(path) = resource { + let mut test_suite = TestSuite::new(path); + case.extra + .insert("line".into(), start.line_number.get().to_string().into()); + case.extra.insert( + "column".into(), + start.column_number.get().to_string().into(), + ); + test_suite.extra.insert("package".into(), "org.pgls".into()); + test_suite.add_test_case(case); + report.add_test_suite(test_suite); + } + } } -} -pub(crate) struct JunitReporterVisitor<'a>(pub(crate) Report, pub(crate) &'a mut dyn Console); - -impl<'a> JunitReporterVisitor<'a> { - pub(crate) fn new(console: &'a mut dyn Console) -> Self { - let report = Report::new("PostgresTools"); - Self(report, console) - } + Ok(()) } -impl ReporterVisitor for JunitReporterVisitor<'_> { - fn report_summary( - &mut self, - _execution: &Execution, - summary: TraversalSummary, - ) -> io::Result<()> { - self.0.time = Some(summary.duration); - self.0.errors = summary.errors as usize; - - Ok(()) +fn should_emit(config: &ReportConfig, diagnostic: &Error) -> bool { + if diagnostic.severity() < config.diagnostic_level { + return false; } - fn report_diagnostics( - &mut self, - _execution: &Execution, - payload: DiagnosticsPayload, - ) -> io::Result<()> { - let diagnostics = payload.diagnostics.iter().filter(|diagnostic| { - if diagnostic.tags().is_verbose() { - payload.verbose - } else { - true - } - }); - - for diagnostic in diagnostics { - let mut status = TestCaseStatus::non_success(NonSuccessKind::Failure); - let message = format!("{}", JunitDiagnostic { diagnostic }); - status.set_message(message.clone()); - - let location = diagnostic.location(); - - if let (Some(span), Some(source_code), Some(resource)) = - (location.span, location.source_code, location.resource) - { - let source = SourceFile::new(source_code); - let start = source.location(span.start())?; - - status.set_description(format!( - "line {row:?}, col {col:?}, {body}", - row = start.line_number.to_zero_indexed(), - col = start.column_number.to_zero_indexed(), - body = message - )); - let mut case = TestCase::new( - format!( - "org.pgt.{}", - diagnostic - .category() - .map(|c| c.name()) - .unwrap_or_default() - .replace('/', ".") - ), - status, - ); - - if let Resource::File(path) = resource { - let mut test_suite = TestSuite::new(path); - case.extra - .insert("line".into(), start.line_number.get().to_string().into()); - case.extra.insert( - "column".into(), - start.column_number.get().to_string().into(), - ); - test_suite.extra.insert("package".into(), "org.pgt".into()); - test_suite.add_test_case(case); - self.0.add_test_suite(test_suite); - } - } - } + if diagnostic.tags().is_verbose() { + config.verbose + } else { + true + } +} - self.1.log(markup! { - {self.0.to_string().unwrap()} - }); +struct JunitDiagnostic<'a> { + diagnostic: &'a Error, +} - Ok(()) +impl Display for JunitDiagnostic<'_> { + fn fmt(&self, fmt: &mut Formatter<'_>) -> std::fmt::Result { + self.diagnostic.description(fmt) } } diff --git a/crates/pgt_cli/src/reporter/mod.rs b/crates/pgt_cli/src/reporter/mod.rs index ed265f9b0..78b8cf1b2 100644 --- a/crates/pgt_cli/src/reporter/mod.rs +++ b/crates/pgt_cli/src/reporter/mod.rs @@ -3,61 +3,147 @@ pub(crate) mod gitlab; pub(crate) mod junit; pub(crate) mod terminal; -use crate::execute::Execution; +use crate::cli_options::{CliOptions, CliReporter}; +use crate::diagnostics::CliDiagnostic; +use pgt_console::Console; use pgt_diagnostics::{Error, Severity}; use pgt_fs::PgTPath; -use serde::Serialize; use std::collections::BTreeSet; -use std::io; +use std::path::PathBuf; use std::time::Duration; -pub struct DiagnosticsPayload { - pub diagnostics: Vec, +#[derive(Debug, Clone)] +pub struct ReportConfig { + pub mode: ReportMode, pub verbose: bool, pub diagnostic_level: Severity, + pub error_on_warnings: bool, + pub no_errors_on_unmatched: bool, +} + +impl ReportConfig { + pub fn from_cli_options(cli_options: &CliOptions) -> Self { + Self { + mode: cli_options.reporter.clone().into(), + verbose: cli_options.verbose, + diagnostic_level: cli_options.diagnostic_level, + error_on_warnings: cli_options.error_on_warnings, + no_errors_on_unmatched: cli_options.no_errors_on_unmatched, + } + } } -/// A type that holds the result of the traversal -#[derive(Debug, Default, Serialize, Copy, Clone)] -pub struct TraversalSummary { +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ReportMode { + Terminal, + GitHub, + GitLab, + Junit, +} + +impl From for ReportMode { + fn from(value: CliReporter) -> Self { + match value { + CliReporter::Default => Self::Terminal, + CliReporter::GitHub => Self::GitHub, + CliReporter::Junit => Self::Junit, + CliReporter::GitLab => Self::GitLab, + } + } +} + +#[derive(Debug)] +pub struct TraversalData { + pub evaluated_paths: BTreeSet, pub changed: usize, pub unchanged: usize, pub matches: usize, - // We skip it during testing because the time isn't predictable - #[cfg_attr(debug_assertions, serde(skip))] - pub duration: Duration, - pub errors: u32, - pub warnings: u32, pub skipped: usize, pub suggested_fixes_skipped: u32, pub diagnostics_not_printed: u32, + pub workspace_root: Option, +} + +#[derive(Debug)] +pub struct Report { + pub diagnostics: Vec, + pub duration: Duration, + pub errors: u32, + pub warnings: u32, + pub skipped_diagnostics: u32, + pub traversal: Option, } -/// When using this trait, the type that implements this trait is the one that holds the read-only information to pass around -pub trait Reporter: Sized { - /// Writes the summary using the underling visitor - fn write(self, visitor: &mut dyn ReporterVisitor) -> io::Result<()>; +impl Report { + pub fn new( + diagnostics: Vec, + duration: Duration, + skipped_diagnostics: u32, + traversal: Option, + ) -> Self { + let (errors, warnings) = count_levels(&diagnostics); + Self { + diagnostics, + duration, + errors, + warnings, + skipped_diagnostics, + traversal, + } + } } -/// When using this trait, the type that implements this trait is the one that will **write** the data, ideally inside a buffer -pub trait ReporterVisitor { - /// Writes the summary in the underling writer - fn report_summary( +pub trait ReportWriter { + fn write( &mut self, - execution: &Execution, - summary: TraversalSummary, - ) -> io::Result<()>; - - /// Writes the paths that were handled during a run. - fn report_handled_paths(&mut self, evaluated_paths: BTreeSet) -> io::Result<()> { - let _ = evaluated_paths; - Ok(()) + console: &mut dyn Console, + command_name: &str, + payload: &Report, + config: &ReportConfig, + ) -> Result<(), CliDiagnostic>; +} + +pub struct Reporter { + config: ReportConfig, +} + +impl Reporter { + pub fn from_cli_options(cli_options: &CliOptions) -> Self { + Self { + config: ReportConfig::from_cli_options(cli_options), + } } - /// Writes a diagnostics - fn report_diagnostics( + pub fn new(config: ReportConfig) -> Self { + Self { config } + } + + pub fn report( &mut self, - execution: &Execution, - payload: DiagnosticsPayload, - ) -> io::Result<()>; + console: &mut dyn Console, + command_name: &str, + payload: &Report, + ) -> Result<(), CliDiagnostic> { + let mut writer: Box = match self.config.mode { + ReportMode::Terminal => Box::new(terminal::TerminalReportWriter), + ReportMode::GitHub => Box::new(github::GithubReportWriter), + ReportMode::GitLab => Box::new(gitlab::GitLabReportWriter), + ReportMode::Junit => Box::new(junit::JunitReportWriter), + }; + + writer.write(console, command_name, payload, &self.config) + } +} + +fn count_levels(diagnostics: &[Error]) -> (u32, u32) { + let mut errors = 0u32; + let mut warnings = 0u32; + for diag in diagnostics { + match diag.severity() { + Severity::Error | Severity::Fatal => errors += 1, + Severity::Warning => warnings += 1, + _ => {} + } + } + (errors, warnings) } diff --git a/crates/pgt_cli/src/reporter/terminal.rs b/crates/pgt_cli/src/reporter/terminal.rs index 6e10efc84..beb84ca51 100644 --- a/crates/pgt_cli/src/reporter/terminal.rs +++ b/crates/pgt_cli/src/reporter/terminal.rs @@ -1,34 +1,84 @@ -use crate::Reporter; -use crate::execute::{Execution, TraversalMode}; -use crate::reporter::{DiagnosticsPayload, ReporterVisitor, TraversalSummary}; +use crate::diagnostics::CliDiagnostic; +use crate::reporter::{Report, ReportConfig, ReportWriter, TraversalData}; use pgt_console::fmt::Formatter; use pgt_console::{Console, ConsoleExt, fmt, markup}; use pgt_diagnostics::advice::ListAdvice; -use pgt_diagnostics::{Diagnostic, PrintDiagnostic}; +use pgt_diagnostics::{Diagnostic, Error, PrintDiagnostic}; use pgt_fs::PgTPath; +use std::borrow::Cow; use std::collections::BTreeSet; -use std::io; -use std::time::Duration; - -pub(crate) struct ConsoleReporter { - pub(crate) summary: TraversalSummary, - pub(crate) diagnostics_payload: DiagnosticsPayload, - pub(crate) execution: Execution, - pub(crate) evaluated_paths: BTreeSet, -} -impl Reporter for ConsoleReporter { - fn write(self, visitor: &mut dyn ReporterVisitor) -> io::Result<()> { - let verbose = self.diagnostics_payload.verbose; - visitor.report_diagnostics(&self.execution, self.diagnostics_payload)?; - visitor.report_summary(&self.execution, self.summary)?; - if verbose { - visitor.report_handled_paths(self.evaluated_paths)?; +pub(crate) struct TerminalReportWriter; + +impl ReportWriter for TerminalReportWriter { + fn write( + &mut self, + console: &mut dyn Console, + command_name: &str, + report: &Report, + config: &ReportConfig, + ) -> Result<(), CliDiagnostic> { + log_diagnostics(console, config, &report.diagnostics); + + if let Some(traversal) = &report.traversal { + console.log(markup! { + {ConsoleTraversalSummary(command_name, report, traversal)} + }); + if config.verbose { + log_evaluated_paths(console, &traversal.evaluated_paths); + } + } else { + console.log(markup! { + {ConsoleDiagnosticSummary(command_name, report)} + }); } + Ok(()) } } +fn log_diagnostics(console: &mut dyn Console, config: &ReportConfig, diagnostics: &[Error]) { + for diagnostic in diagnostics { + if diagnostic.severity() < config.diagnostic_level { + continue; + } + + if diagnostic.tags().is_verbose() && config.verbose { + console.error(markup! {{PrintDiagnostic::verbose(diagnostic)}}); + } else if !diagnostic.tags().is_verbose() { + console.error(markup! {{PrintDiagnostic::simple(diagnostic)}}); + } + } +} + +fn log_evaluated_paths(console: &mut dyn Console, evaluated_paths: &BTreeSet) { + let evaluated_paths_diagnostic = EvaluatedPathsDiagnostic { + advice: ListAdvice { + list: evaluated_paths + .iter() + .map(|p| p.display().to_string()) + .collect(), + }, + }; + + let fixed_paths_diagnostic = FixedPathsDiagnostic { + advice: ListAdvice { + list: evaluated_paths + .iter() + .filter(|p| p.was_written()) + .map(|p| p.display().to_string()) + .collect(), + }, + }; + + console.log(markup! { + {PrintDiagnostic::verbose(&evaluated_paths_diagnostic)} + }); + console.log(markup! { + {PrintDiagnostic::verbose(&fixed_paths_diagnostic)} + }); +} + #[derive(Debug, Diagnostic)] #[diagnostic( tags(VERBOSE), @@ -51,76 +101,10 @@ struct FixedPathsDiagnostic { advice: ListAdvice, } -pub(crate) struct ConsoleReporterVisitor<'a>(pub(crate) &'a mut dyn Console); - -impl ReporterVisitor for ConsoleReporterVisitor<'_> { - fn report_summary( - &mut self, - execution: &Execution, - summary: TraversalSummary, - ) -> io::Result<()> { - self.0.log(markup! { - {ConsoleTraversalSummary(execution.traversal_mode(), &summary)} - }); - - Ok(()) - } - - fn report_handled_paths(&mut self, evaluated_paths: BTreeSet) -> io::Result<()> { - let evaluated_paths_diagnostic = EvaluatedPathsDiagnostic { - advice: ListAdvice { - list: evaluated_paths - .iter() - .map(|p| p.display().to_string()) - .collect(), - }, - }; - - let fixed_paths_diagnostic = FixedPathsDiagnostic { - advice: ListAdvice { - list: evaluated_paths - .iter() - .filter(|p| p.was_written()) - .map(|p| p.display().to_string()) - .collect(), - }, - }; - - self.0.log(markup! { - {PrintDiagnostic::verbose(&evaluated_paths_diagnostic)} - }); - self.0.log(markup! { - {PrintDiagnostic::verbose(&fixed_paths_diagnostic)} - }); - - Ok(()) - } - - fn report_diagnostics( - &mut self, - _execution: &Execution, - diagnostics_payload: DiagnosticsPayload, - ) -> io::Result<()> { - for diagnostic in &diagnostics_payload.diagnostics { - if diagnostic.severity() >= diagnostics_payload.diagnostic_level { - if diagnostic.tags().is_verbose() && diagnostics_payload.verbose { - self.0 - .error(markup! {{PrintDiagnostic::verbose(diagnostic)}}); - } else { - self.0 - .error(markup! {{PrintDiagnostic::simple(diagnostic)}}); - } - } - } - - Ok(()) - } -} - struct Files(usize); impl fmt::Display for Files { - fn fmt(&self, fmt: &mut Formatter) -> io::Result<()> { + fn fmt(&self, fmt: &mut Formatter) -> std::io::Result<()> { fmt.write_markup(markup!({self.0} " "))?; if self.0 == 1 { fmt.write_str("file") @@ -133,7 +117,7 @@ impl fmt::Display for Files { struct SummaryDetail(usize); impl fmt::Display for SummaryDetail { - fn fmt(&self, fmt: &mut Formatter) -> io::Result<()> { + fn fmt(&self, fmt: &mut Formatter) -> std::io::Result<()> { if self.0 > 0 { fmt.write_markup(markup! { " Fixed "{Files(self.0)}"." @@ -145,46 +129,81 @@ impl fmt::Display for SummaryDetail { } } } -struct SummaryTotal<'a>(&'a TraversalMode, usize, &'a Duration); - -impl fmt::Display for SummaryTotal<'_> { - fn fmt(&self, fmt: &mut Formatter) -> io::Result<()> { - let files = Files(self.1); - match self.0 { - TraversalMode::Dummy => fmt.write_markup(markup! { - "Dummy "{files}" in "{self.2}"." - }), - TraversalMode::Check { .. } => fmt.write_markup(markup! { - "Checked "{files}" in "{self.2}"." - }), - } - } -} -pub(crate) struct ConsoleTraversalSummary<'a>( - pub(crate) &'a TraversalMode, - pub(crate) &'a TraversalSummary, -); +struct ConsoleTraversalSummary<'a>(&'a str, &'a Report, &'a TraversalData); + impl fmt::Display for ConsoleTraversalSummary<'_> { - fn fmt(&self, fmt: &mut Formatter) -> io::Result<()> { - let summary = SummaryTotal(self.0, self.1.changed + self.1.unchanged, &self.1.duration); - let detail = SummaryDetail(self.1.changed); - fmt.write_markup(markup!({summary}{detail}))?; + fn fmt(&self, fmt: &mut Formatter) -> std::io::Result<()> { + let action = traversal_action(self.0); + let files = Files(self.2.changed + self.2.unchanged); + fmt.write_markup(markup!( + + {action} " "{files}" in "{self.1.duration}"." {SummaryDetail(self.2.changed)} + + ))?; if self.1.errors > 0 { if self.1.errors == 1 { - fmt.write_markup(markup!("\n""Found "{self.1.errors}" error."))?; + fmt.write_markup(markup!( + "\n""Found "{self.1.errors}" error." + ))?; } else { - fmt.write_markup(markup!("\n""Found "{self.1.errors}" errors."))?; + fmt.write_markup(markup!( + "\n""Found "{self.1.errors}" errors." + ))?; } } if self.1.warnings > 0 { if self.1.warnings == 1 { - fmt.write_markup(markup!("\n""Found "{self.1.warnings}" warning."))?; + fmt.write_markup(markup!( + "\n""Found "{self.1.warnings}" warning." + ))?; } else { - fmt.write_markup(markup!("\n""Found "{self.1.warnings}" warnings."))?; + fmt.write_markup(markup!( + "\n""Found "{self.1.warnings}" warnings." + ))?; } } Ok(()) } } + +struct ConsoleDiagnosticSummary<'a>(&'a str, &'a Report); + +impl fmt::Display for ConsoleDiagnosticSummary<'_> { + fn fmt(&self, fmt: &mut Formatter) -> std::io::Result<()> { + let action = diagnostic_action(self.0); + fmt.write_markup(markup! { + + {action} " completed in "{self.1.duration}"." + + })?; + + if self.1.errors > 0 { + fmt.write_markup(markup!( + "\n""Found "{self.1.errors}" error(s)." + ))?; + } + + if self.1.warnings > 0 { + fmt.write_markup(markup!( + "\n""Found "{self.1.warnings}" warning(s)." + ))?; + } + Ok(()) + } +} + +fn traversal_action(command: &str) -> Cow<'static, str> { + match command { + "check" => Cow::Borrowed("Checked"), + _ => Cow::Borrowed("Processed"), + } +} + +fn diagnostic_action(command: &str) -> Cow<'static, str> { + match command { + "check" => Cow::Borrowed("Check"), + _ => Cow::Borrowed("Command"), + } +} diff --git a/crates/pgt_cli/src/workspace.rs b/crates/pgt_cli/src/workspace.rs new file mode 100644 index 000000000..13cc7722e --- /dev/null +++ b/crates/pgt_cli/src/workspace.rs @@ -0,0 +1,48 @@ +use crate::{CliDiagnostic, VcsIntegration}; +use pgt_configuration::{ConfigurationPathHint, PartialConfiguration}; +use pgt_fs::FileSystem; +use pgt_workspace::PartialConfigurationExt; +use pgt_workspace::configuration::{LoadedConfiguration, load_configuration}; +use pgt_workspace::workspace::{RegisterProjectFolderParams, UpdateSettingsParams}; +use pgt_workspace::{DynRef, Workspace}; + +/// Load configuration from disk and emit warnings for deprecated filenames. +pub fn load_config( + fs: &DynRef<'_, dyn FileSystem>, + config_hint: ConfigurationPathHint, +) -> Result { + load_configuration(fs, config_hint).map_err(CliDiagnostic::from) +} + +/// Configure the workspace and VCS integration according to the provided configuration. +pub fn setup_workspace( + workspace: &dyn Workspace, + fs: &DynRef<'_, dyn FileSystem>, + configuration: PartialConfiguration, + vcs: VcsIntegration, +) -> Result<(), CliDiagnostic> { + let (vcs_base_path, gitignore_matches) = match vcs { + VcsIntegration::Enabled => configuration + .retrieve_gitignore_matches(fs, fs.working_directory().as_deref()) + .map_err(CliDiagnostic::from)?, + VcsIntegration::Disabled => (None, vec![]), + }; + + workspace + .register_project_folder(RegisterProjectFolderParams { + path: fs.working_directory(), + set_as_current_workspace: true, + }) + .map_err(CliDiagnostic::from)?; + + workspace + .update_settings(UpdateSettingsParams { + workspace_directory: fs.working_directory(), + configuration, + vcs_base_path, + gitignore_matches, + }) + .map_err(CliDiagnostic::from)?; + + Ok(()) +} diff --git a/crates/pgt_cli/tests/snapshots/assert_check__check_junit_reporter_snapshot.snap b/crates/pgt_cli/tests/snapshots/assert_check__check_junit_reporter_snapshot.snap index 243b265cc..046b2e506 100644 --- a/crates/pgt_cli/tests/snapshots/assert_check__check_junit_reporter_snapshot.snap +++ b/crates/pgt_cli/tests/snapshots/assert_check__check_junit_reporter_snapshot.snap @@ -6,9 +6,9 @@ snapshot_kind: text status: failure stdout: - - - + + + line 0, col 0, Invalid statement: syntax error at or near "tqjable" From 888356d8831724abdfec9042d880053ebaddca33 Mon Sep 17 00:00:00 2001 From: psteinroe Date: Tue, 28 Oct 2025 11:34:03 +0100 Subject: [PATCH 3/4] progress --- PLAN.md | 968 -------------------------------------------------------- 1 file changed, 968 deletions(-) delete mode 100644 PLAN.md diff --git a/PLAN.md b/PLAN.md deleted file mode 100644 index 9a473d227..000000000 --- a/PLAN.md +++ /dev/null @@ -1,968 +0,0 @@ -# CLI Architecture Refactor Plan - -## Current Problems - -### 1. CommandRunner Trait Forces All Commands Through Same Pipeline -```rust -pub(crate) trait CommandRunner: Sized { - fn run(&mut self, session: CliSession, cli_options: &CliOptions) -> Result<()> { - // Forces: config loading → VCS setup → file traversal → reporting - let (execution, paths) = self.configure_workspace(fs, console, workspace, cli_options)?; - execute_mode(execution, session, cli_options, paths) - } -} -``` - -**Issues:** -- Only `Check` command uses this trait -- Commands like `version`, `clean`, `init` are ad-hoc functions -- No way for commands to skip parts they don't need (e.g., dblint needs config but not traversal) -- Forces unnecessary complexity on simple commands - -### 2. TraversalMode::Dummy Exists For Wrong Reasons -```rust -pub enum TraversalMode { - Dummy, // Only exists because commands are forced through traversal - Check { stdin: Option, vcs_targeted: VcsTargeted }, -} -``` - -Commands that don't need traversal shouldn't have a "dummy" mode - they just shouldn't traverse. - -### 3. Execution Struct Conflates Concerns -```rust -pub struct Execution { - report_mode: ReportMode, // How to report (Terminal/GitHub/GitLab) - traversal_mode: TraversalMode, // How to process files - max_diagnostics: u32, -} -``` - -**Problem:** Bundles processing config with reporting config. Commands that don't traverse (like `dblint`) still need reporting but don't need traversal config. - -### 4. execute_mode() is Monolithic -```rust -pub fn execute_mode(execution: Execution, session: CliSession, ...) -> Result<()> { - // Does: stdin handling + traversal + reporting + exit codes - // Can't be reused by commands that don't traverse -} -``` - -### 5. Scattered and Inconsistent Structure -- `commands/` has mix of trait impls and functions -- `execute/` has traversal logic tightly coupled to reporting -- No clear separation of workspace setup, execution, and reporting -- Commands repeat boilerplate for config loading, setup, reporting - ---- - -## Proposed Clean Architecture - -### Three Command Tiers - -We have three distinct types of commands: - -**Tier 1: Simple** - No workspace needed -- `version`, `clean`, `stop`, `print_socket` -- Just need Console/FileSystem - -**Tier 2: Workspace-only** - Config/workspace, no file traversal -- `init`, `dblint` -- Load config, setup workspace, analyze, report - -**Tier 3: Full Pipeline** - Config + workspace + file traversal -- `check`, (future: `format`, `lint`) -- Load config, setup workspace with VCS, traverse files, report - -### New Architecture Layers - -``` -┌─────────────────────────────────────────┐ -│ commands/ │ -│ Simple functions, no forced trait │ -│ - version, clean, init, dblint, check │ -└─────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────┐ -│ CliSession with Helper Methods │ -│ - prepare_with_config() │ -│ - setup_workspace(VcsIntegration) │ -│ - report() │ -└─────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────┐ -│ workspace.rs (NEW) │ -│ - load_config() │ -│ - setup_workspace(..., VcsIntegration) │ -└─────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────┐ -│ execute/ (RESTRUCTURED) │ -│ - ExecutionConfig + helpers │ -│ - run_files() │ -│ - run_stdin() │ -│ Clear split between walker & stdin │ -└─────────────────────────────────────────┘ - ↓ -┌─────────────────────────────────────────┐ -│ reporter/ │ -│ - Reporter struct + Report enum │ -│ Works with ANY diagnostic source │ -└─────────────────────────────────────────┘ -``` - ---- - -## Detailed Design - -### 1. CliSession Helper Methods - -Add helper methods to reduce boilerplate: - -```rust -// crates/pgt_cli/src/lib.rs - -impl<'app> CliSession<'app> { - /// Helper accessors - pub fn fs(&self) -> &DynRef<'_, dyn FileSystem> { - &self.app.fs - } - - pub fn console(&mut self) -> &mut dyn Console { - &mut *self.app.console - } - - pub fn workspace(&self) -> &dyn Workspace { - &*self.app.workspace - } - - /// Common setup: logging + config loading + merging - pub fn prepare_with_config( - &mut self, - cli_options: &CliOptions, - cli_configuration: Option, - ) -> Result { - setup_cli_subscriber(cli_options.log_level, cli_options.log_kind); - - // Take borrows once so immutable/mutable access do not overlap - let fs = self.fs(); - let console = self.console(); - let loaded = workspace::load_config( - fs, - console, - cli_options.as_configuration_path_hint(), - )?; - - let mut configuration = loaded.configuration; - if let Some(cli_config) = cli_configuration { - configuration.merge_with(cli_config); - } - - Ok(configuration) - } - - /// Setup workspace with optional VCS integration - pub fn setup_workspace( - &mut self, - configuration: PartialConfiguration, - vcs: VcsIntegration, - ) -> Result<(), CliDiagnostic> { - let workspace = self.workspace(); - let fs = self.fs(); - - workspace::setup_workspace(workspace, fs, configuration, vcs) - } - - /// Report results (common final step) - pub fn report( - &mut self, - cli_options: &CliOptions, - report: Report, - ) -> Result<(), CliDiagnostic> { - let mut reporter = Reporter::from_cli_options(cli_options); - reporter.report(self.console(), report) - } -} - -/// Controls whether workspace setup includes VCS integration -pub enum VcsIntegration { - /// Enable VCS integration (gitignore, changed files detection) - Enabled, - /// Skip VCS integration - Disabled, -} -} -``` - -### 2. Workspace Setup Layer (NEW) - -```rust -// crates/pgt_cli/src/workspace.rs - NEW FILE - -use crate::VcsIntegration; -use pgt_configuration::PartialConfiguration; -use pgt_console::{Console, ConsoleExt, markup}; -use pgt_fs::{ConfigName, FileSystem}; -use pgt_workspace::{DynRef, Workspace}; -use pgt_workspace::configuration::{LoadedConfiguration, load_configuration}; -use pgt_workspace::workspace::{RegisterProjectFolderParams, UpdateSettingsParams}; -use std::path::PathBuf; - -/// Load configuration from filesystem with deprecation warning -pub fn load_config( - fs: &DynRef<'_, dyn FileSystem>, - console: &mut dyn Console, - config_hint: Option<&PathBuf>, -) -> Result { - let loaded = load_configuration(fs, config_hint)?; - - if let Some(config_path) = &loaded.file_path { - if let Some(file_name) = config_path.file_name().and_then(|n| n.to_str()) { - if ConfigName::is_deprecated(file_name) { - console.log(markup! { - "Warning: ""Deprecated config filename. Use 'postgres-language-server.jsonc'.\n" - }); - } - } - } - - Ok(loaded) -} - -pub fn setup_workspace( - workspace: &dyn Workspace, - fs: &DynRef<'_, dyn FileSystem>, - configuration: PartialConfiguration, - vcs: VcsIntegration, -) -> Result<(), CliDiagnostic> { - let (vcs_base_path, gitignore_matches) = match vcs { - VcsIntegration::Enabled => configuration - .retrieve_gitignore_matches(fs, fs.working_directory().as_deref()) - .map_err(CliDiagnostic::from)?, - VcsIntegration::Disabled => (None, vec![]), - }; - - workspace - .register_project_folder(RegisterProjectFolderParams { - path: fs.working_directory(), - set_as_current_workspace: true, - }) - .map_err(CliDiagnostic::from)?; - - workspace - .update_settings(UpdateSettingsParams { - workspace_directory: fs.working_directory(), - configuration, - vcs_base_path, - gitignore_matches, - }) - .map_err(CliDiagnostic::from)?; - - Ok(()) -} -``` - -### 3. execute/ Module (restructured, not new) - -We keep the familiar `execute/` name but make the responsibilities crystal clear: - -```text -execute/ - mod.rs - public entry points (run_files, run_stdin) + ExecutionConfig - config.rs - ExecutionMode / FixMode / VcsTargeting helpers - walk.rs - filesystem traversal + VCS-aware path filtering - stdin.rs - converts stdin payload into process_file jobs - process_file/ - unchanged, per-file logic -``` - -#### Execution configuration lives in `execute/config.rs` - -```rust -// Shared across files + stdin paths -pub struct ExecutionConfig { - pub mode: ExecutionMode, - pub limits: ExecutionLimits, -} - -pub enum ExecutionMode { - Check { fix_mode: Option, vcs: VcsTargeting }, - Format { write: bool, ignore_errors: bool, vcs: VcsTargeting }, - Lint { only: Vec, skip: Vec, fix_mode: Option, vcs: VcsTargeting }, -} - -pub enum FixMode { - Safe, - Suggested, -} - -pub struct VcsTargeting { - pub staged: bool, - pub changed: bool, -} - -pub struct ExecutionLimits { - pub max_diagnostics: u32, - pub allow_writes: bool, -} - -impl ExecutionConfig { - pub fn new(mode: ExecutionMode, max_diagnostics: u32) -> Self { - let allow_writes = matches!( - &mode, - ExecutionMode::Check { fix_mode: Some(_), .. } - | ExecutionMode::Format { write: true, .. } - | ExecutionMode::Lint { fix_mode: Some(_), .. } - ); - Self { - mode, - limits: ExecutionLimits { - max_diagnostics, - allow_writes, - }, - } - } -} -``` - -Having a single config object means commands share one type regardless of whether they operate on stdin or directories. `allow_writes` is derived from the mode so CLI commands can gate destructive actions early. - -#### Public helpers stay tiny - -```rust -pub fn run_files( - session: &mut CliSession, - config: &ExecutionConfig, - paths: Vec, -) -> Result { - walk::traverse(session, config, paths) -} - -pub fn run_stdin( - session: &mut CliSession, - config: &ExecutionConfig, - payload: StdinPayload, -) -> Result { - stdin::process(session, config, payload) -} - -pub struct StdinPayload { - pub path: PathBuf, - pub content: String, -} -``` - -Where `StdinPayload` is a simple `{ path: PathBuf, content: String }` struct defined next to the helpers. - -#### Reporting structures stay outside execute/ - -`execute/` no longer defines the old `DiagnosticResult` / `TraversalResult` structs. Instead it emits neutral `ExecutionSummary` values (counts, changed/skipped, collected diagnostics). The reusable `DiagnosticSummary` type that dblint also needs lives under `reporter/summary.rs` (see next section). This keeps execute/ focused on producing data, not on how it will be displayed. - -```rust -// crates/pgt_cli/src/reporter/mod.rs - ADD REPORT CONFIG - -/// Configuration for how to report results -/// SEPARATE from traversal configuration -#[derive(Debug, Clone)] -pub struct ReportConfig { - pub mode: ReportMode, - pub verbose: bool, - pub diagnostic_level: DiagnosticLevel, - pub error_on_warnings: bool, - pub no_errors_on_unmatched: bool, -} - -impl ReportConfig { - pub fn from_cli_options(cli_options: &CliOptions) -> Self { - Self { - mode: cli_options.reporter.clone().into(), - verbose: cli_options.verbose, - diagnostic_level: cli_options.diagnostic_level, - error_on_warnings: cli_options.error_on_warnings, - no_errors_on_unmatched: cli_options.no_errors_on_unmatched, - } - } -} - -/// Summaries live with the reporter so non-traversal commands (dblint) can reuse them -pub struct DiagnosticSummary { - pub diagnostics: Vec, - pub duration: Duration, - pub errors: u32, - pub warnings: u32, -} - -impl DiagnosticSummary { - pub fn from_diagnostics(diagnostics: Vec, duration: Duration) -> Self { - let errors = diagnostics.iter().filter(|d| d.severity().is_error()).count() as u32; - let warnings = diagnostics.iter().filter(|d| d.severity().is_warning()).count() as u32; - Self { diagnostics, duration, errors, warnings } - } -} - -pub struct ExecutionSummary { - pub diagnostics: DiagnosticSummary, - pub evaluated_paths: Vec, - pub changed: usize, - pub unchanged: usize, - pub skipped: usize, -} - -/// User-facing type describing what should be reported -pub enum Report { - Diagnostics { - summary: DiagnosticSummary, - command_name: &'static str, - }, - Traversal { - summary: ExecutionSummary, - command_name: &'static str, - }, -} - -impl Report { - pub fn from_diagnostic_summary( - command_name: &'static str, - summary: DiagnosticSummary, - ) -> Self { - Report::Diagnostics { summary, command_name } - } - - pub fn from_execution_summary( - command_name: &'static str, - summary: ExecutionSummary, - ) -> Self { - Report::Traversal { summary, command_name } - } -} - -/// Reporter struct instead of bare function calls -pub struct Reporter { - config: ReportConfig, -} - -impl Reporter { - pub fn from_cli_options(cli_options: &CliOptions) -> Self { - Self { - config: ReportConfig::from_cli_options(cli_options), - } - } - - pub fn report( - &mut self, - console: &mut dyn Console, - report: Report, - ) -> Result<(), CliDiagnostic> { - match report { - Report::Diagnostics { summary, command_name } => { - self.report_diagnostics(console, summary, command_name) - } - Report::Traversal { summary, command_name } => { - self.report_traversal(console, summary, command_name) - } - } - } - - fn report_diagnostics( - &self, - console: &mut dyn Console, - summary: DiagnosticSummary, - command_name: &str, - ) -> Result<(), CliDiagnostic> { - // existing logic from report_analysis - } - - fn report_traversal( - &self, - console: &mut dyn Console, - summary: ExecutionSummary, - command_name: &str, - ) -> Result<(), CliDiagnostic> { - // existing logic from report_traversal - } -} - -// Implementation note: the bodies of `report_diagnostics` and `report_traversal` -// reuse today's `report_analysis` / `report_traversal` logic verbatim, just moved -// behind the struct methods so callers always use the same entry point. -``` - -### 4. Example Commands - -#### Tier 1: Simple Command (No Config) - -```rust -// crates/pgt_cli/src/commands/version.rs - -pub fn version(mut session: CliSession) -> Result<(), CliDiagnostic> { - session.console().log(markup! { - "CLI: "{VERSION} - }); - - match session.workspace().server_info() { - None => { - session.console().log(markup! { - "Server: ""not connected" - }); - } - Some(info) => { - session.console().log(markup! { - "Server: - Name: "{info.name}" - Version: "{info.version.unwrap_or_else(|| "-".to_string())} - }); - } - }; - - Ok(()) -} -``` - -#### Tier 2: Config-Only Command (No Traversal) - -```rust -// crates/pgt_cli/src/commands/dblint.rs - -pub fn dblint( - mut session: CliSession, - cli_options: &CliOptions, - cli_configuration: Option, -) -> Result<(), CliDiagnostic> { - // Step 1: Common setup (logging + config loading + merging) - let configuration = session.prepare_with_config(cli_options, cli_configuration)?; - - // Step 2: Setup workspace (no VCS needed) - session.setup_workspace(configuration.clone(), VcsIntegration::Disabled)?; - - // Step 3: Custom analysis logic (no file traversal!) - let start = Instant::now(); - let diagnostics = analyze_workspace_config(session.workspace(), &configuration)?; - let duration = start.elapsed(); - - // Step 4: Build summary (clean - no fake traversal data!) - let summary = DiagnosticSummary::from_diagnostics(diagnostics, duration); - - // Step 5: Report (same infrastructure as check) - session.report( - cli_options, - Report::from_diagnostic_summary("dblint", summary), - ) -} - -fn analyze_workspace_config( - workspace: &dyn Workspace, - config: &PartialConfiguration, -) -> Result, CliDiagnostic> { - // Your dblint analysis logic here - // Example: validate linting rules, check for conflicts, etc. - let mut diagnostics = vec![]; - - if let Some(linter_config) = &config.linter { - if linter_config.rules.is_empty() { - diagnostics.push( - Error::from_message("No linting rules configured") - .with_severity(Severity::Warning) - .with_category(category!("dblint")) - ); - } - } - - Ok(diagnostics) -} - -``` - -#### Tier 3: Full Pipeline Command (Traversal) - -```rust -// crates/pgt_cli/src/commands/check.rs - -pub struct CheckArgs { - pub configuration: Option, - pub paths: Vec, - pub stdin_file_path: Option, - pub staged: bool, - pub changed: bool, - pub since: Option, - pub apply: bool, - pub apply_unsafe: bool, -} - -pub fn check( - mut session: CliSession, - cli_options: &CliOptions, - args: CheckArgs, -) -> Result<(), CliDiagnostic> { - // Step 1: Common setup - let configuration = session.prepare_with_config(cli_options, args.configuration)?; - validate_args(&args)?; - - // Step 2: Setup workspace with VCS (needed for traversal) - session.setup_workspace(configuration.clone(), VcsIntegration::Enabled)?; - - // Step 3: Compute paths (from CLI args, VCS, or default) - let paths = compute_paths(session.fs(), &configuration, &args)?; - - // Step 4: Build execution mode - let mode = ExecutionMode::Check { - fix_mode: if args.apply_unsafe { - Some(FixMode::Suggested) - } else if args.apply { - Some(FixMode::Safe) - } else { - None - }, - vcs: VcsTargeting { - staged: args.staged, - changed: args.changed, - }, - }; - - // Step 5: Build config once and reuse it regardless of the input source - let max_diagnostics = if cli_options.reporter.is_default() { - cli_options.max_diagnostics - } else { - u32::MAX - }; - let execution_config = ExecutionConfig::new(mode, max_diagnostics); - - let summary = if let Some(stdin_path) = args.stdin_file_path { - let content = session.console().read() - .ok_or_else(|| CliDiagnostic::missing_argument("stdin", "check"))?; - - execute::run_stdin( - &mut session, - &execution_config, - StdinPayload { - path: PathBuf::from(stdin_path), - content, - }, - )? - } else { - execute::run_files(&mut session, &execution_config, paths)? - }; - - // Step 6: Report - session.report( - cli_options, - Report::from_execution_summary("check", summary), - ) -} - -fn validate_args(args: &CheckArgs) -> Result<(), CliDiagnostic> { - if args.since.is_some() && !args.changed { - return Err(CliDiagnostic::incompatible_arguments("since", "changed")); - } - if args.changed && args.staged { - return Err(CliDiagnostic::incompatible_arguments("changed", "staged")); - } - Ok(()) -} - -fn compute_paths( - fs: &DynRef<'_, dyn FileSystem>, - configuration: &PartialConfiguration, - args: &CheckArgs, -) -> Result, CliDiagnostic> { - if args.changed { - get_changed_files(fs, configuration, args.since.as_deref()) - } else if args.staged { - get_staged_files(fs) - } else if !args.paths.is_empty() { - Ok(args.paths.clone()) - } else { - Ok(vec![]) // Default to current directory (handled inside execute::run_files) - } -} -``` - -### 5. Execution Flow - -Here's how the "full pipeline" path works in the check command: - -``` -check command - ↓ -execute::run_files(&mut session, &config, paths) - ↓ -walk::traverse(session, config, paths) - ↓ -Walk file system from starting paths - ↓ -For each file found: - ↓ -process_file::process_file(options, path) - ↓ (dispatches based on mode) - ↓ -For Check mode: - workspace.pull_diagnostics() - runs linting -For Format mode: - workspace.format() - formats file -For Lint mode: - workspace.pull_diagnostics() with specific rules - ↓ -Send diagnostics to collector thread - ↓ -After traversal completes: - ↓ -Return ExecutionSummary { diagnostics, counts } - ↓ -check command → Reporter::report(...) -``` - -**Key insight:** The `execute/` module is **mode-agnostic** - it works with Check, Format, Lint, etc. Commands like `dblint`, `version`, `init` never import it because they don't process files. - -### 6. Updated lib.rs - -```rust -// crates/pgt_cli/src/lib.rs - -mod cli_options; -mod commands; -mod diagnostics; -mod logging; -mod reporter; -mod execute; // RESTRUCTURED - replaces old execute/ -mod workspace; // NEW - -impl<'app> CliSession<'app> { - // ... helper methods defined above ... - - pub fn run(self, command: PgtCommand) -> Result<(), CliDiagnostic> { - match command { - PgtCommand::Version(_) => commands::version::version(self), - PgtCommand::Clean => commands::clean::clean(self), - PgtCommand::Init => commands::init::init(self), - - PgtCommand::Dblint { cli_options, configuration } => { - commands::dblint::dblint(self, &cli_options, configuration) - } - - PgtCommand::Check { - cli_options, - configuration, - paths, - stdin_file_path, - staged, - changed, - since, - } => { - commands::check::check( - self, - &cli_options, - commands::check::CheckArgs { - configuration, - paths, - stdin_file_path, - staged, - changed, - since, - apply: false, // Add these flags to PgtCommand enum - apply_unsafe: false, - }, - ) - } - - // ... other commands - } - } -} - -// REMOVE: run_command() helper -// REMOVE: CommandRunner trait -``` - ---- - -## Comparison: Before vs After - -| Aspect | Current | Proposed | -|--------|---------|----------| -| **Command structure** | Mix of CommandRunner trait + ad-hoc functions | All functions, no forced trait | -| **Simple commands** | Ad-hoc implementations | Clean functions using helpers | -| **Config-only commands** | No good pattern | `prepare_with_config()` + `setup_workspace(VcsIntegration::Disabled)` | -| **Workspace setup** | `setup_full_workspace()` vs `setup_minimal_workspace()` (unclear) | `setup_workspace(config, VcsIntegration::Enabled/Disabled)` (clear) | -| **Result types** | TraversalSummary used even when not traversing | `DiagnosticSummary` for analysis, `ExecutionSummary` for traversal | -| **Traversal commands** | Forced through CommandRunner | Explicit `execute::run_files()` call | -| **Code reuse** | Trait with mandatory methods | Helper methods on CliSession | -| **Traversal modes** | Has Dummy mode | No Dummy - traversal is opt-in | -| **Execution bundle** | Conflates processing + reporting | Split: ExecutionConfig + ReportConfig | -| **Reporting** | Coupled to execute_mode | `Reporter` struct with `Report` payloads | -| **Boilerplate** | High - repeated config loading | Low - helpers reduce repetition | -| **Flexibility** | Low - forced through pipeline | High - compose what you need | -| **Clarity** | Unclear what commands need | Crystal clear from function calls | - ---- - -## Pros and Cons - -### Pros - -✅ **Clear command tiers** - Easy to understand what each command needs -✅ **No forced abstractions** - Commands use only what they need -✅ **ExecutionMode preserved** - Keeps rich configuration like Biome -✅ **No Dummy mode** - Traversal is truly optional -✅ **Separation of concerns** - Processing config separate from reporting config -✅ **Reusable infrastructure** - Reporting works for all commands -✅ **Reduced boilerplate** - Helper methods eliminate repetition -✅ **Extensible** - Easy to add new modes (Format, Lint) or commands -✅ **Testable** - Each layer can be tested independently -✅ **Explicit data flow** - No hidden trait magic -✅ **Flexible reporting** - Any command can generate diagnostics and use same reporting -✅ **Clear workspace setup** - `VcsIntegration::Enabled/Disabled` is self-documenting -✅ **Proper result types** - DiagnosticSummary vs ExecutionSummary - no fake data - -### Cons - -⚠️ **More methods on CliSession** - Could become bloated if too many helpers added -⚠️ **No enforced structure** - Commands could skip important steps if not careful -⚠️ **Duplication risk** - Without trait, commands might duplicate logic (mitigated by helpers) -⚠️ **Learning curve** - New contributors need to understand helper methods -⚠️ **No compile-time guarantees** - Can't enforce "must setup workspace before traversal" - -### Mitigation Strategies - -- Keep CliSession helpers focused on truly common patterns -- Document command implementation patterns clearly -- Use helper function modules (like workspace.rs) for shared logic -- Add integration tests that verify correct command flow -- Add focused helper functions (e.g., `ensure_workspace_ready`) if we need stronger guidance later - ---- - -## Migration Path - -### Phase 1: Setup Foundation -1. Create `crates/pgt_cli/src/workspace.rs` with helper functions -2. Add helper methods to `CliSession` (accessors + borrow-safe `prepare_with_config` + `setup_workspace`) -3. Split `Execution` into `ExecutionConfig` and `ReportConfig` -4. Introduce `Reporter` struct (`from_cli_options` + `report()`) - -### Phase 2: Migrate Simple Commands -1. Refactor `version` to use new helpers (already simple) -2. Refactor `clean` to use new helpers -3. Smoke-test both commands - -### Phase 3: Add Dblint (Config-Only Command) -1. Implement `commands/dblint.rs` using `prepare_with_config()` + `setup_workspace(...Disabled)` -2. Implement `analyze_workspace_config()` logic -3. Add dblint to `PgtCommand` enum and `CliSession::run()` -4. Test dblint command - -### Phase 4: Migrate Check Command -1. Restructure `execute/` (config.rs, walk.rs, stdin.rs, process_file/) -2. Add `execute::run_files()` helper (thin wrapper over walk) -3. Add `execute::run_stdin()` helper (buffer path) -4. Update `walk.rs` to use `ExecutionConfig` -5. Ensure walker no longer depends on stdin specifics -6. Refactor `check.rs` to call `run_stdin`/`run_files` -7. Test check command thoroughly (traversal, stdin, VCS modes) - -### Phase 5: Cleanup -1. Remove `CommandRunner` trait from `commands/mod.rs` -2. Remove `execute_mode()` function -3. Remove `TraversalMode::Dummy` -4. Remove old `Execution` struct -5. Update imports to new `execute::{self, config, stdin}` layout -6. Update/extend tests -7. Update documentation - -### Phase 6: Future Commands -1. Add `format` command using ExecutionMode::Format -2. Add `lint` command using ExecutionMode::Lint -3. Each new command follows established patterns - ---- - -## File Changes Summary - -### Files to Create/Rename -- `crates/pgt_cli/src/workspace.rs` - NEW - Workspace setup helpers -- `crates/pgt_cli/src/commands/dblint.rs` - NEW - Dblint command - -### Files to Modify (Significant) -- `crates/pgt_cli/src/lib.rs` - Add helper methods to CliSession, simplify run() -- `crates/pgt_cli/src/execute/mod.rs` - Export `ExecutionConfig`, `run_files`, `run_stdin` -- `crates/pgt_cli/src/execute/config.rs` - NEW internal module for modes/limits -- `crates/pgt_cli/src/execute/walk.rs` - Rewritten to use `ExecutionConfig` -- `crates/pgt_cli/src/execute/stdin.rs` - Buffer path feeding process_file -- `crates/pgt_cli/src/reporter/mod.rs` - Add ReportConfig, `Reporter` struct, `Report` enum, summaries -- `crates/pgt_cli/src/commands/check.rs` - Call `run_stdin`/`run_files` -- `crates/pgt_cli/src/commands/mod.rs` - Remove CommandRunner trait (~135 lines) -- `crates/pgt_cli/src/execute/process_file/mod.rs` - Dispatch based on ExecutionMode - -### Files to Modify (Minor) -- `crates/pgt_cli/src/commands/version.rs` - Use helper methods -- `crates/pgt_cli/src/commands/clean.rs` - Use helper methods -- `crates/pgt_cli/src/execute/process_file/*.rs` - Use ExecutionMode from config -- Remove the old `execute/std_in.rs` and fold logic into `execute/stdin.rs` - -### Lines of Code Impact -- **Delete:** ~200 lines (CommandRunner trait, execute_mode monolith, Dummy mode) -- **Add:** ~300 lines (workspace.rs, helpers, dblint, configs) -- **Net:** +100 lines but much clearer structure - ---- - -## Key Design Principles - -1. **Opt-in over forced** - Commands choose what they need, not forced through pipeline -2. **Separation of concerns** - Config, traversal, reporting are independent -3. **Explicit over implicit** - Function calls show what's happening, no hidden trait magic -4. **Simple stays simple** - Don't force complexity on commands that don't need it -5. **Reusable infrastructure** - Reporting works for any diagnostic source -6. **Composable helpers** - Small, focused helper methods instead of monolithic abstractions -7. **Rich configuration** - ExecutionMode captures "how" to process files, keep it flexible -8. **Clear boundaries** - `execute/` owns runtime pipeline, stdin/files are explicit helpers - -### Why keep `execute/` but reshape it? - -**Original pain:** `execute/` mixed config, traversal, stdin, and reporting glue. You had to understand everything to touch anything. - -**New structure:** -``` -execute/ - mod.rs - `ExecutionConfig`, `run_files`, `run_stdin` - config.rs - ExecutionMode + FixMode + limits - walk.rs - directory walking + VCS targeting - stdin.rs - buffer path, no filesystem assumptions - process_file/ - unchanged processing logic -``` - -Now the name still matches the CLI command (`pgt execute check`), but the responsibilities are sliced: commands talk to `run_*`, the walker never sees stdin, and reporting lives elsewhere. - ---- - -## Open Questions - -1. **Should we add more optional helpers?** e.g., `compute_vcs_paths()`, `read_stdin()` - - Lean towards yes, but only after seeing repeated patterns - -2. **Should ReportConfig handle category name or commands pass it?** - - Currently commands pass command_name - keeps reporting generic - -3. **Do we need a WorkspaceConfig struct?** - - Currently passing PartialConfiguration directly - works but could bundle with paths - -4. **Should stdin handling stay inside execute/?** - - Current plan keeps `run_stdin()` next to `run_files()` for shared logic, but we can split later if it grows complex - ---- - -## Success Criteria - -✅ All three command tiers have clear, distinct implementations -✅ No TraversalMode::Dummy -✅ Dblint command works without traversal -✅ Check command works with traversal (files, stdin, VCS modes) -✅ All reporters (Terminal, GitHub, GitLab, JUnit) work for all commands -✅ Less boilerplate in command implementations -✅ All existing tests pass -✅ Code is easier to understand for new contributors - ---- - -## References - -- Biome CLI architecture - Good example of rich execution modes -- Current `pgt_cli` implementation - Understanding existing patterns -- Rust API guidelines - General API ergonomics and error handling guidance From 99c226400e3088053979fbc5f89f6847cb38148a Mon Sep 17 00:00:00 2001 From: psteinroe Date: Tue, 28 Oct 2025 16:16:20 +0100 Subject: [PATCH 4/4] progress --- crates/pgt_cli/src/commands/check.rs | 3 --- crates/pgt_cli/src/execute/process_file/workspace_file.rs | 5 ++++- crates/pgt_cli/src/execute/walk.rs | 2 ++ crates/pgt_cli/src/lib.rs | 2 -- crates/pgt_cli/src/reporter/gitlab.rs | 7 +++++++ 5 files changed, 13 insertions(+), 6 deletions(-) diff --git a/crates/pgt_cli/src/commands/check.rs b/crates/pgt_cli/src/commands/check.rs index 4d00b9adf..e5017f8b8 100644 --- a/crates/pgt_cli/src/commands/check.rs +++ b/crates/pgt_cli/src/commands/check.rs @@ -11,7 +11,6 @@ use pgt_fs::FileSystem; use pgt_workspace::DynRef; use std::ffi::OsString; -#[allow(dead_code)] pub struct CheckArgs { pub configuration: Option, pub paths: Vec, @@ -19,8 +18,6 @@ pub struct CheckArgs { pub staged: bool, pub changed: bool, pub since: Option, - pub apply: bool, - pub apply_unsafe: bool, } pub fn check( diff --git a/crates/pgt_cli/src/execute/process_file/workspace_file.rs b/crates/pgt_cli/src/execute/process_file/workspace_file.rs index 5236ff40e..58475b566 100644 --- a/crates/pgt_cli/src/execute/process_file/workspace_file.rs +++ b/crates/pgt_cli/src/execute/process_file/workspace_file.rs @@ -9,8 +9,11 @@ use std::path::{Path, PathBuf}; /// Small wrapper that holds information and operations around the current processed file pub(crate) struct WorkspaceFile<'ctx, 'app> { guard: FileGuard<'app, dyn Workspace + 'ctx>, + /// File handle for the underlying filesystem entry, if backed by a real file. + /// Not present for stdin execution where content is provided as a temporary buffer. + /// Currently unused but will be needed when autofix/write operations are implemented. #[allow(dead_code)] - file: Option>, // only present when backed by a real fs entry + file: Option>, pub(crate) path: PathBuf, } diff --git a/crates/pgt_cli/src/execute/walk.rs b/crates/pgt_cli/src/execute/walk.rs index bb156e9a1..915b9738a 100644 --- a/crates/pgt_cli/src/execute/walk.rs +++ b/crates/pgt_cli/src/execute/walk.rs @@ -161,8 +161,10 @@ fn traverse_inputs( struct DiagnosticsPrinter<'ctx> { _config: &'ctx ExecutionConfig, + /// The maximum number of diagnostics the console thread is allowed to print max_diagnostics: u32, remaining_diagnostics: AtomicU32, + /// Count of diagnostics that exceeded max_diagnostics and weren't printed not_printed_diagnostics: AtomicU32, printed_diagnostics: AtomicU32, total_skipped_suggested_fixes: AtomicU32, diff --git a/crates/pgt_cli/src/lib.rs b/crates/pgt_cli/src/lib.rs index c350293be..f8d609d9d 100644 --- a/crates/pgt_cli/src/lib.rs +++ b/crates/pgt_cli/src/lib.rs @@ -87,8 +87,6 @@ impl<'app> CliSession<'app> { staged, changed, since, - apply: false, - apply_unsafe: false, }, ), PgtCommand::Clean => commands::clean::clean(self), diff --git a/crates/pgt_cli/src/reporter/gitlab.rs b/crates/pgt_cli/src/reporter/gitlab.rs index 1d89db492..d5a4b60de 100644 --- a/crates/pgt_cli/src/reporter/gitlab.rs +++ b/crates/pgt_cli/src/reporter/gitlab.rs @@ -139,12 +139,19 @@ impl Display for GitLabDiagnostics<'_> { } } +/// An entry in the GitLab Code Quality report. +/// See https://docs.gitlab.com/ee/ci/testing/code_quality.html#implement-a-custom-tool #[derive(Serialize)] pub struct GitLabDiagnostic<'a> { + /// A description of the code quality violation. description: String, + /// A unique name representing the static analysis check that emitted this issue. check_name: &'a str, + /// A unique fingerprint to identify the code quality violation. For example, an MD5 hash. fingerprint: String, + /// A severity string (can be info, minor, major, critical, or blocker). severity: &'a str, + /// The location where the code quality violation occurred. location: Location, }