refactor(cli): simplify abstractions (#574)

this PR refactors the `cli` crate. it currently has a bunch of issues that stem from its original implementation in biome, where file/stdin execution is the main use case. - a `CommandRunner` trait that tries to abstract a few common code blocks away. - a `Dummy` traversal mode which essentially means "do not traverse". - reports are tightly coupled to execution. we also need to report output of commands that do not traverse (e.g. `dblint`) - `execute_mode` does everything: stdin handling + traversal + reporting + exit codes. it cannot be re-used by commands that do not traverse all of these issues cause my brain to have a hard time understanding what is going on. This PR refactors the crate to gear it more towards our use-case. I spent a few cycles designing the refactor, and at least for me it makes much more sense now. I also added a dummy `dblint` command that will be implemented in follow-ups. right now, it prints an empty report. Below is a richer summary of the issues described above. ## 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<Stdin>, 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

Author: psteinroe

crates/pgt_cli/src/commands/check.rs

@@ -1,76 +1,138 @@
 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};
+pub struct CheckArgs {
+ pub configuration: Option<PartialConfiguration>,
+ pub paths: Vec<OsString>,
+ pub stdin_file_path: Option<String>,
+ pub staged: bool,
+ pub changed: bool,
+ pub since: Option<String>,
+}
+
+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<PartialConfiguration>,
- pub(crate) paths: Vec<OsString>,
- pub(crate) stdin_file_path: Option<String>,
- pub(crate) staged: bool,
- pub(crate) changed: bool,
- pub(crate) since: Option<String>,
+ 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<PartialConfiguration, WorkspaceError> {
- 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<Vec<OsString>, 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<StdinPayload, CliDiagnostic> {
+ 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<Vec<OsString>, 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<Execution, CliDiagnostic> {
- 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(())
 }

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<PartialConfiguration>,
+) -> 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)
+}