From 2ab2f9201555b7200bf05ff40f9a45ce66c594ce Mon Sep 17 00:00:00 2001 From: Rain Date: Thu, 14 May 2026 23:28:19 -0700 Subject: [PATCH 1/3] [spr] initial version Created using spr 1.3.6-beta.1 --- AGENTS.md | 22 +- crates/dropshot-api-manager/src/cmd/debug.rs | 2 +- .../dropshot-api-manager/src/cmd/generate.rs | 54 +-- crates/dropshot-api-manager/src/output.rs | 114 +++-- crates/dropshot-api-manager/src/resolved.rs | 390 +++++++++++------- 5 files changed, 365 insertions(+), 217 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index c360a0f..d22a1c6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -56,14 +56,14 @@ Every Rust source file must start with: - **Newtypes** for domain types (e.g., `ApiIdent`, `VcsRevision`, `GitCommitHash`, `ApiSpecFileName`) - **Builder patterns** for complex construction (e.g., `ManagedApi` with `with_extra_validation`, `with_git_stub_storage`) - **Type states** encoded in generics when state transitions matter -- **Lifetimes** used extensively to avoid cloning (e.g., `Problem<'a>`, `Resolution<'a>`, `Fix<'a>`) +- **Lifetimes** used extensively to avoid cloning (e.g., `VersionProblem<'a>`, `Resolution<'a>`, `Fix<'a>`) - **Restricted visibility**: Use `pub(crate)` and `pub(super)` liberally - **Non-exhaustive**: Consider forward compatibility for public error types ### Error handling - Use `thiserror` for error types with `#[derive(Error)]`. -- Group errors by category with specific enums (e.g., `Problem`, `Note`). +- Group errors by category with specific enums (e.g., `VersionProblem`, `NonVersionProblem`, `Note`). - Provide rich error context using `anyhow::Context`. - Two-tier model: - **Problems**: User-visible issues that may be fixable or unfixable. @@ -179,17 +179,27 @@ pub enum ResolutionKind { NewLocally, // Versioned, only in current branch. } -pub enum Problem<'a> { - LocalSpecFileOrphaned { ... }, +// Per-(api, version) problems: anything tied to a specific supported +// version. The bulk of the variants live here. +pub enum VersionProblem<'a> { BlessedVersionMissingLocal { ... }, BlessedVersionBroken { ... }, LockstepStale { ... }, LocalVersionMissingLocal { ... }, // ... more variants } + +// Problems not tied to a specific version: orphaned files, unparseable +// files, and the "latest" symlink for versioned APIs. +pub enum NonVersionProblem<'a> { + LocalSpecFileOrphaned { ... }, + UnparseableLocalFile { ... }, + LatestLinkMissing { ... }, + LatestLinkStale { ... }, +} ``` -Problems are either **fixable** (tool can auto-correct) or **unfixable** (require manual intervention). +Both problem enums are either **fixable** (tool can auto-correct) or **unfixable** (require manual intervention). Splitting per-version problems out lets rendering code that needs per-(api, version) context (e.g., compatibility-issue dedup) require it at the type level. ### Key design principles @@ -213,7 +223,7 @@ crates/ │ │ ├── apis.rs # ManagedApiConfig, ManagedApi, ManagedApis │ │ ├── cmd/ # CLI commands (dispatch, check, generate, list, debug) │ │ ├── environment.rs # Environment configuration and resolution -│ │ ├── resolved.rs # Resolution logic, Problem enum, Fix enum +│ │ ├── resolved.rs # Resolution logic, VersionProblem / NonVersionProblem / Fix enums │ │ ├── compatibility.rs # Wire compatibility checking via drift │ │ ├── validation.rs # OpenAPI document validation │ │ ├── vcs/ # VCS abstraction (RepoVcs: Git/Jujutsu dispatch) diff --git a/crates/dropshot-api-manager/src/cmd/debug.rs b/crates/dropshot-api-manager/src/cmd/debug.rs index 1195997..f611d83 100644 --- a/crates/dropshot-api-manager/src/cmd/debug.rs +++ b/crates/dropshot-api-manager/src/cmd/debug.rs @@ -59,7 +59,7 @@ pub(crate) fn debug_impl( for note in resolved.notes() { println!("NOTE: {}", note); } - for problem in resolved.general_problems() { + for problem in resolved.non_version_problems() { println!("PROBLEM: {}", problem); } diff --git a/crates/dropshot-api-manager/src/cmd/generate.rs b/crates/dropshot-api-manager/src/cmd/generate.rs index e209db6..ff2e6b0 100644 --- a/crates/dropshot-api-manager/src/cmd/generate.rs +++ b/crates/dropshot-api-manager/src/cmd/generate.rs @@ -6,11 +6,12 @@ use crate::{ environment::{BlessedSource, GeneratedSource, ResolvedEnv}, output::{ CheckResult, OutputOpts, Styles, display_api_spec_version, - display_load_problems, display_resolution, display_resolution_problems, + display_load_problems, display_non_version_problems, + display_resolution, display_version_problems, headers::{self, *}, plural, }, - resolved::{Problem, Resolved}, + resolved::{Fix, NonVersionProblem, Resolved, VersionProblem}, }; use anyhow::{Result, anyhow, bail}; use owo_colors::OwoColorize; @@ -135,10 +136,10 @@ fn generate_impl_inner( display_api_spec_version(api, version, styles, resolution), )?; - fix_problems( + apply_fixes( writer, env, - problems, + problems.into_iter().map(expect_version_fix), styles, &mut num_updated, &mut num_errors, @@ -154,10 +155,10 @@ fn generate_impl_inner( ident.style(styles.filename), )?; - fix_problems( + apply_fixes( writer, env, - std::iter::once(symlink_problem), + std::iter::once(expect_non_version_fix(symlink_problem)), styles, &mut num_updated, &mut num_errors, @@ -173,11 +174,10 @@ fn generate_impl_inner( } // Fix problems not associated with any supported version, if any. - let general_problems: Vec<_> = resolved.general_problems().collect(); - fix_problems( + apply_fixes( writer, env, - general_problems, + resolved.non_version_problems().map(expect_non_version_fix), styles, &mut num_updated, &mut num_errors, @@ -217,10 +217,11 @@ fn generate_impl_inner( display_load_problems(writer, &errors, styles)?; let resolved = Resolved::new(env, apis, &blessed, &generated, &local_files_recheck); - let general_problems: Vec<_> = resolved.general_problems().collect(); - nproblems += general_problems.len(); - if !general_problems.is_empty() { - display_resolution_problems(writer, env, general_problems, styles)?; + let non_version_problems: Vec<_> = + resolved.non_version_problems().collect(); + nproblems += non_version_problems.len(); + if !non_version_problems.is_empty() { + display_non_version_problems(writer, non_version_problems, styles)?; } for api in apis.iter_apis() { let ident = api.ident(); @@ -237,7 +238,7 @@ fn generate_impl_inner( (this is a bug)", ident, version )?; - display_resolution_problems(writer, env, problems, styles)?; + display_version_problems(writer, env, problems, styles)?; } } @@ -248,9 +249,8 @@ fn generate_impl_inner( "found unexpected problem with API {} symlink (this is a bug)", ident )?; - display_resolution_problems( + display_non_version_problems( writer, - env, std::iter::once(symlink_problem), styles, )?; @@ -314,21 +314,23 @@ fn print_final_status( Ok(()) } -fn fix_problems<'a, T>( +fn expect_version_fix<'a>(p: &'a VersionProblem<'a>) -> Fix<'a> { + p.fix().expect("attempting to fix unfixable problem") +} + +fn expect_non_version_fix<'a>(p: &'a NonVersionProblem<'a>) -> Fix<'a> { + p.fix().expect("attempting to fix unfixable problem") +} + +fn apply_fixes<'a>( writer: &mut dyn io::Write, env: &ResolvedEnv, - problems: T, + fixes: impl IntoIterator>, styles: &Styles, num_updated: &mut usize, num_errors: &mut usize, -) -> io::Result<()> -where - T: IntoIterator>, -{ - for p in problems { - // We should have already bailed out if there were any unfixable - // problems. - let fix = p.fix().expect("attempting to fix unfixable problem"); +) -> io::Result<()> { + for fix in fixes { match fix.execute(env) { Ok(steps) => { *num_updated += 1; diff --git a/crates/dropshot-api-manager/src/output.rs b/crates/dropshot-api-manager/src/output.rs index 00084c3..992704d 100644 --- a/crates/dropshot-api-manager/src/output.rs +++ b/crates/dropshot-api-manager/src/output.rs @@ -4,7 +4,9 @@ use crate::{ FAILURE_EXIT_CODE, NEEDS_UPDATE_EXIT_CODE, apis::{ManagedApi, ManagedApis}, environment::{ErrorAccumulator, ResolvedEnv}, - resolved::{Problem, Resolution, ResolutionKind, Resolved}, + resolved::{ + NonVersionProblem, Resolution, ResolutionKind, Resolved, VersionProblem, + }, validation::CheckStale, }; use anyhow::bail; @@ -294,7 +296,7 @@ pub fn display_resolution( let mut num_fresh = 0; let mut num_stale = 0; let mut num_failed = 0; - let mut num_general_problems = 0; + let mut num_non_version_problems = 0; // Print problems associated with a supported API version // (i.e., one of the expected OpenAPI documents). @@ -321,16 +323,15 @@ pub fn display_resolution( if let Some(symlink_problem) = resolved.symlink_problem(ident) { if symlink_problem.is_fixable() { - num_general_problems += 1; + num_non_version_problems += 1; writeln!( writer, "{:>HEADER_WIDTH$} {} \"latest\" symlink", STALE.style(styles.warning_header), ident.style(styles.filename), )?; - display_resolution_problems( + display_non_version_problems( writer, - env, std::iter::once(symlink_problem), styles, )?; @@ -342,9 +343,8 @@ pub fn display_resolution( FAILURE.style(styles.failure_header), ident.style(styles.filename), )?; - display_resolution_problems( + display_non_version_problems( writer, - env, std::iter::once(symlink_problem), styles, )?; @@ -361,8 +361,9 @@ pub fn display_resolution( } // Print problems not associated with any supported version, if any. - let general_problems: Vec<_> = resolved.general_problems().collect(); - num_general_problems += if !general_problems.is_empty() { + let non_version_problems: Vec<_> = + resolved.non_version_problems().collect(); + num_non_version_problems += if !non_version_problems.is_empty() { writeln!( writer, "\n{:>HEADER_WIDTH$} problems not associated with a specific \ @@ -370,10 +371,12 @@ pub fn display_resolution( "Other".style(styles.warning_header), )?; - let (fixable, unfixable): (Vec<&Problem>, Vec<&Problem>) = - general_problems.iter().partition(|p| p.is_fixable()); + let (fixable, unfixable): ( + Vec<&NonVersionProblem>, + Vec<&NonVersionProblem>, + ) = non_version_problems.iter().partition(|p| p.is_fixable()); num_failed += unfixable.len(); - display_resolution_problems(writer, env, general_problems, styles)?; + display_non_version_problems(writer, non_version_problems, styles)?; fixable.len() } else { 0 @@ -399,7 +402,7 @@ pub fn display_resolution( // Print a summary line. let status_header = if num_failed > 0 { FAILURE.style(styles.failure_header) - } else if num_stale > 0 || num_general_problems > 0 { + } else if num_stale > 0 || num_non_version_problems > 0 { STALE.style(styles.warning_header) } else { SUCCESS.style(styles.success_header) @@ -416,8 +419,8 @@ pub fn display_resolution( num_fresh.style(styles.bold), num_stale.style(styles.bold), num_failed.style(styles.bold), - num_general_problems.style(styles.bold), - plural::problems(num_general_problems), + num_non_version_problems.style(styles.bold), + plural::problems(num_non_version_problems), )?; if num_failed > 0 { writeln!( @@ -427,7 +430,7 @@ pub fn display_resolution( format!("{} generate", env.command).style(styles.bold) )?; Ok(CheckResult::Failures) - } else if num_stale > 0 || num_general_problems > 0 { + } else if num_stale > 0 || num_non_version_problems > 0 { writeln!( writer, "{:>HEADER_WIDTH$} (run {} to update)", @@ -496,20 +499,21 @@ fn summarize_one( display_api_spec_version(api, version, styles, resolution), )?; - display_resolution_problems(writer, env, problems, styles)?; + display_version_problems(writer, env, problems, styles)?; } Ok(()) } -/// Print a formatted list of Problems to `writer`. -pub fn display_resolution_problems<'a, T>( +/// Print a formatted list of per-(api, version) [`VersionProblem`]s to +/// `writer`, including any compatibility-issue diffs and fix descriptions. +pub fn display_version_problems<'a, T>( writer: &mut dyn io::Write, env: &ResolvedEnv, problems: T, styles: &Styles, ) -> io::Result<()> where - T: IntoIterator>, + T: IntoIterator>, { for p in problems.into_iter() { let subheader_width = HEADER_WIDTH + 4; @@ -535,7 +539,9 @@ where // For BlessedVersionBroken, print each item separately, along with a // diff between blessed and generated versions. - if let Problem::BlessedVersionBroken { compatibility_issues } = &p { + if let VersionProblem::BlessedVersionBroken { compatibility_issues } = + &p + { for issue in compatibility_issues { // Print each compatibility issue on a new line, prefixed with // "- ". @@ -578,7 +584,7 @@ where // For BlessedLatestVersionBytewiseMismatch, show a diff between blessed // and generated versions even though there's no fix. - if let Problem::BlessedLatestVersionBytewiseMismatch { + if let VersionProblem::BlessedLatestVersionBytewiseMismatch { blessed, generated, } = p @@ -627,7 +633,7 @@ where // When possible, print a useful diff of changes. let do_diff = match p { - Problem::LockstepStale { found, generated } => { + VersionProblem::LockstepStale { found, generated } => { let diff = TextDiff::from_lines( found.contents(), generated.contents(), @@ -639,7 +645,7 @@ where .join(generated.spec_file_name().path()); Some((diff, path1, path2)) } - Problem::ExtraFileStale { + VersionProblem::ExtraFileStale { check_stale: CheckStale::Modified { full_path, actual, expected }, .. @@ -647,7 +653,7 @@ where let diff = TextDiff::from_lines(actual, expected); Some((diff, full_path.clone(), full_path.clone())) } - Problem::LocalVersionStale { spec_files, generated } + VersionProblem::LocalVersionStale { spec_files, generated } if spec_files.len() == 1 => { let diff = TextDiff::from_lines( @@ -685,6 +691,64 @@ where Ok(()) } +/// Print a formatted list of [`NonVersionProblem`]s (orphaned files, +/// unparseable files, and "latest" symlink issues) to `writer`. None of +/// these variants have associated diffs, so this is just header + fix. +pub fn display_non_version_problems<'a, T>( + writer: &mut dyn io::Write, + problems: T, + styles: &Styles, +) -> io::Result<()> +where + T: IntoIterator>, +{ + for p in problems.into_iter() { + let subheader_width = HEADER_WIDTH + 4; + let first_indent = format!( + "{:>subheader_width$}: ", + if p.is_fixable() { + "problem".style(styles.warning_header) + } else { + "error".style(styles.failure_header) + } + ); + let more_indent = " ".repeat(subheader_width + 2); + writeln!( + writer, + "{}", + textwrap::fill( + &InlineErrorChain::new(&p).to_string(), + textwrap::Options::with_termwidth() + .initial_indent(&first_indent) + .subsequent_indent(&more_indent) + ) + )?; + + let Some(fix) = p.fix() else { + continue; + }; + + let first_indent = format!( + "{:>subheader_width$}: ", + "fix".style(styles.warning_header) + ); + let fix_str = fix.to_string(); + for s in fix_str.trim_end().split("\n") { + writeln!( + writer, + "{}", + textwrap::fill( + &format!("will {}", s), + textwrap::Options::with_termwidth() + .initial_indent(&first_indent) + .subsequent_indent(&more_indent) + ) + )?; + } + } + Ok(()) +} + /// Adapter for [`Error`]s that provides a [`std::fmt::Display`] implementation /// that print the full chain of error sources, separated by `: `. pub struct InlineErrorChain<'a>(&'a dyn std::error::Error); diff --git a/crates/dropshot-api-manager/src/resolved.rs b/crates/dropshot-api-manager/src/resolved.rs index 60b4137..b9e3865 100644 --- a/crates/dropshot-api-manager/src/resolved.rs +++ b/crates/dropshot-api-manager/src/resolved.rs @@ -73,19 +73,21 @@ pub enum Note { /// and local spec files for a particular API pub struct Resolution<'a> { kind: ResolutionKind, - problems: Vec>, + problems: Vec>, } impl<'a> Resolution<'a> { - pub fn new_lockstep(problems: Vec>) -> Resolution<'a> { + pub fn new_lockstep(problems: Vec>) -> Resolution<'a> { Resolution { kind: ResolutionKind::Lockstep, problems } } - pub fn new_blessed(problems: Vec>) -> Resolution<'a> { + pub fn new_blessed(problems: Vec>) -> Resolution<'a> { Resolution { kind: ResolutionKind::Blessed, problems } } - pub fn new_new_locally(problems: Vec>) -> Resolution<'a> { + pub fn new_new_locally( + problems: Vec>, + ) -> Resolution<'a> { Resolution { kind: ResolutionKind::NewLocally, problems } } @@ -94,7 +96,7 @@ impl<'a> Resolution<'a> { } /// Add a problem to this resolution. - pub fn add_problem(&mut self, problem: Problem<'a>) { + pub fn add_problem(&mut self, problem: VersionProblem<'a>) { self.problems.push(problem); } @@ -102,7 +104,9 @@ impl<'a> Resolution<'a> { self.problems().any(|p| !p.is_fixable()) } - pub fn problems(&self) -> impl Iterator> + '_ { + pub fn problems( + &self, + ) -> impl Iterator> + '_ { self.problems.iter() } @@ -132,10 +136,12 @@ impl Display for ResolutionKind { } } -/// Identifies the kind of a `Problem` without carrying borrowed data. +/// Identifies the kind of a `VersionProblem` or `NonVersionProblem` without +/// carrying borrowed data. /// -/// Each variant corresponds 1:1 to a `Problem` variant. The exhaustive -/// match in `Problem::kind` ensures that adding a new `Problem` variant +/// Each variant corresponds 1:1 to a variant of one of the two problem +/// enums. Exhaustive matches in `VersionProblem::kind` and +/// `NonVersionProblem::kind` ensure that adding a new problem variant /// without updating this enum causes a compile error. #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)] #[expect(missing_docs)] @@ -165,7 +171,8 @@ pub enum ProblemKind { GitStubFirstCommitUnknown, } -/// Owned summary of a `Problem` for test assertions. +/// Owned summary of a `VersionProblem` or `NonVersionProblem` for test +/// assertions. /// /// Contains just enough information to identify a problem: which API it /// belongs to, which version (if any), and its [`ProblemKind`]. Because all @@ -203,11 +210,15 @@ impl ProblemSummary { } } -/// Describes a problem resolving the blessed spec(s), generated spec(s), and -/// local spec files for a particular API. +/// Describes a problem with the blessed spec(s), generated spec(s), and local +/// spec files for an API that is *not* tied to a specific supported version. +/// +/// Per-(api, version) problems live in [`VersionProblem`] instead. Splitting +/// the two means callers can prove at compile time that, e.g., the symlink +/// problem slot can never hold a `BlessedVersionBroken`, and rendering code +/// that needs version-specific context can require it unconditionally. #[derive(Debug, Error)] -pub enum Problem<'a> { - // These problems are not associated with any *supported* version of an API. +pub enum NonVersionProblem<'a> { #[error( "A local OpenAPI document was found that does not correspond to a \ supported version of this API: {spec_file_name}. This is unusual, \ @@ -227,8 +238,33 @@ pub enum Problem<'a> { )] UnparseableLocalFile { unparseable_file: UnparseableFile }, - // All other problems are associated with specific supported versions of an - // API. + #[error("\"Latest\" symlink for versioned API {api_ident:?} is missing")] + LatestLinkMissing { + api_ident: ApiIdent, + link: &'a VersionedApiSpecFileName, + }, + + #[error( + "\"Latest\" symlink for versioned API {api_ident:?} is stale: points \ + to {}, but should be {}", + found.basename(), + link.basename(), + )] + LatestLinkStale { + api_ident: ApiIdent, + found: &'a VersionedApiSpecFileName, + link: &'a VersionedApiSpecFileName, + }, +} + +/// Describes a problem with a specific supported (api, version) pair. +/// +/// Companion to [`NonVersionProblem`], which holds problems that aren't tied to a +/// specific version. The split lets rendering code that needs per-(api, +/// version) context (e.g., for compatibility-issue dedup) require that +/// context at the type level. +#[derive(Debug, Error)] +pub enum VersionProblem<'a> { #[error( "This version is blessed, and it's a supported version, but it's \ missing a local OpenAPI document. This can happen with dependent \ @@ -357,24 +393,6 @@ pub enum Problem<'a> { check_stale: CheckStale, }, - #[error("\"Latest\" symlink for versioned API {api_ident:?} is missing")] - LatestLinkMissing { - api_ident: ApiIdent, - link: &'a VersionedApiSpecFileName, - }, - - #[error( - "\"Latest\" symlink for versioned API {api_ident:?} is stale: points \ - to {}, but should be {}", - found.basename(), - link.basename(), - )] - LatestLinkStale { - api_ident: ApiIdent, - found: &'a VersionedApiSpecFileName, - link: &'a VersionedApiSpecFileName, - }, - #[error( "Blessed non-latest version is stored as a full JSON file. This can \ be converted to a Git stub. This tool can perform the conversion for \ @@ -433,68 +451,113 @@ pub enum Problem<'a> { }, } -impl<'a> Problem<'a> { +impl<'a> NonVersionProblem<'a> { /// Returns the discriminant of this problem as a [`ProblemKind`]. /// - /// The match is exhaustive (no wildcard), so adding a new `Problem` + /// The match is exhaustive (no wildcard), so adding a new `NonVersionProblem` /// variant without updating this method causes a compile error. pub fn kind(&self) -> ProblemKind { match self { - Problem::LocalSpecFileOrphaned { .. } => { + NonVersionProblem::LocalSpecFileOrphaned { .. } => { ProblemKind::LocalSpecFileOrphaned } - Problem::UnparseableLocalFile { .. } => { + NonVersionProblem::UnparseableLocalFile { .. } => { ProblemKind::UnparseableLocalFile } - Problem::BlessedVersionMissingLocal { .. } => { + NonVersionProblem::LatestLinkMissing { .. } => { + ProblemKind::LatestLinkMissing + } + NonVersionProblem::LatestLinkStale { .. } => { + ProblemKind::LatestLinkStale + } + } + } + + pub fn is_fixable(&self) -> bool { + self.fix().is_some() + } + + pub fn fix(&'a self) -> Option> { + match self { + NonVersionProblem::LocalSpecFileOrphaned { spec_file_name } => { + Some(Fix::DeleteFiles { + files: DisplayableVec(vec![spec_file_name.clone().into()]), + }) + } + NonVersionProblem::UnparseableLocalFile { unparseable_file } => { + Some(Fix::DeleteUnparseableFile { + path: unparseable_file.path.clone(), + }) + } + NonVersionProblem::LatestLinkStale { api_ident, link, .. } + | NonVersionProblem::LatestLinkMissing { api_ident, link } => { + Some(Fix::UpdateSymlink { api_ident, link }) + } + } + } +} + +impl<'a> VersionProblem<'a> { + /// Returns the discriminant of this problem as a [`ProblemKind`]. + /// + /// The match is exhaustive (no wildcard), so adding a new + /// `VersionProblem` variant without updating this method causes a compile + /// error. + pub fn kind(&self) -> ProblemKind { + match self { + VersionProblem::BlessedVersionMissingLocal { .. } => { ProblemKind::BlessedVersionMissingLocal } - Problem::BlessedVersionExtraLocalSpec { .. } => { + VersionProblem::BlessedVersionExtraLocalSpec { .. } => { ProblemKind::BlessedVersionExtraLocalSpec } - Problem::BlessedVersionCompareError { .. } => { + VersionProblem::BlessedVersionCompareError { .. } => { ProblemKind::BlessedVersionCompareError } - Problem::BlessedVersionBroken { .. } => { + VersionProblem::BlessedVersionBroken { .. } => { ProblemKind::BlessedVersionBroken } - Problem::BlessedLatestVersionBytewiseMismatch { .. } => { + VersionProblem::BlessedLatestVersionBytewiseMismatch { .. } => { ProblemKind::BlessedLatestVersionBytewiseMismatch } - Problem::LockstepMissingLocal { .. } => { + VersionProblem::LockstepMissingLocal { .. } => { ProblemKind::LockstepMissingLocal } - Problem::LockstepStale { .. } => ProblemKind::LockstepStale, - Problem::LocalVersionMissingLocal { .. } => { + VersionProblem::LockstepStale { .. } => ProblemKind::LockstepStale, + VersionProblem::LocalVersionMissingLocal { .. } => { ProblemKind::LocalVersionMissingLocal } - Problem::LocalVersionExtra { .. } => ProblemKind::LocalVersionExtra, - Problem::LocalVersionStale { .. } => ProblemKind::LocalVersionStale, - Problem::GeneratedSourceMissing { .. } => { + VersionProblem::LocalVersionExtra { .. } => { + ProblemKind::LocalVersionExtra + } + VersionProblem::LocalVersionStale { .. } => { + ProblemKind::LocalVersionStale + } + VersionProblem::GeneratedSourceMissing { .. } => { ProblemKind::GeneratedSourceMissing } - Problem::GeneratedValidationError { .. } => { + VersionProblem::GeneratedValidationError { .. } => { ProblemKind::GeneratedValidationError } - Problem::ExtraFileStale { .. } => ProblemKind::ExtraFileStale, - Problem::LatestLinkMissing { .. } => ProblemKind::LatestLinkMissing, - Problem::LatestLinkStale { .. } => ProblemKind::LatestLinkStale, - Problem::BlessedVersionShouldBeGitStub { .. } => { + VersionProblem::ExtraFileStale { .. } => { + ProblemKind::ExtraFileStale + } + VersionProblem::BlessedVersionShouldBeGitStub { .. } => { ProblemKind::BlessedVersionShouldBeGitStub } - Problem::GitStubShouldBeJson { .. } => { + VersionProblem::GitStubShouldBeJson { .. } => { ProblemKind::GitStubShouldBeJson } - Problem::BlessedVersionCorruptedLocal { .. } => { + VersionProblem::BlessedVersionCorruptedLocal { .. } => { ProblemKind::BlessedVersionCorruptedLocal } - Problem::DuplicateLocalFile { .. } => { + VersionProblem::DuplicateLocalFile { .. } => { ProblemKind::DuplicateLocalFile } - Problem::GitStubCommitStale { .. } => { + VersionProblem::GitStubCommitStale { .. } => { ProblemKind::GitStubCommitStale } - Problem::GitStubFirstCommitUnknown { .. } => { + VersionProblem::GitStubFirstCommitUnknown { .. } => { ProblemKind::GitStubFirstCommitUnknown } } @@ -506,36 +569,32 @@ impl<'a> Problem<'a> { pub fn fix(&'a self) -> Option> { match self { - Problem::LocalSpecFileOrphaned { spec_file_name } => { - Some(Fix::DeleteFiles { - files: DisplayableVec(vec![spec_file_name.clone().into()]), - }) - } - Problem::BlessedVersionMissingLocal { blessed, git_stub } => { - Some(Fix::RestoreFromBlessed { - blessed, - git_stub: git_stub.as_ref(), - }) - } - Problem::BlessedVersionExtraLocalSpec { spec_file_name } => { + VersionProblem::BlessedVersionMissingLocal { + blessed, + git_stub, + } => Some(Fix::RestoreFromBlessed { + blessed, + git_stub: git_stub.as_ref(), + }), + VersionProblem::BlessedVersionExtraLocalSpec { spec_file_name } => { Some(Fix::DeleteFiles { files: DisplayableVec(vec![spec_file_name.clone().into()]), }) } - Problem::BlessedVersionCompareError { .. } => None, - Problem::BlessedVersionBroken { .. } => None, - Problem::BlessedLatestVersionBytewiseMismatch { .. } => None, - Problem::LockstepMissingLocal { generated } - | Problem::LockstepStale { generated, .. } => { + VersionProblem::BlessedVersionCompareError { .. } => None, + VersionProblem::BlessedVersionBroken { .. } => None, + VersionProblem::BlessedLatestVersionBytewiseMismatch { .. } => None, + VersionProblem::LockstepMissingLocal { generated } + | VersionProblem::LockstepStale { generated, .. } => { Some(Fix::UpdateLockstepFile { generated }) } - Problem::LocalVersionMissingLocal { generated } => { + VersionProblem::LocalVersionMissingLocal { generated } => { Some(Fix::UpdateVersionedFiles { old: DisplayableVec(Vec::new()), generated, }) } - Problem::LocalVersionExtra { spec_file_names } => { + VersionProblem::LocalVersionExtra { spec_file_names } => { Some(Fix::DeleteFiles { files: DisplayableVec( spec_file_names @@ -547,7 +606,7 @@ impl<'a> Problem<'a> { ), }) } - Problem::LocalVersionStale { spec_files, generated } => { + VersionProblem::LocalVersionStale { spec_files, generated } => { Some(Fix::UpdateVersionedFiles { old: DisplayableVec( spec_files.iter().map(|s| s.spec_file_name()).collect(), @@ -555,22 +614,19 @@ impl<'a> Problem<'a> { generated, }) } - Problem::GeneratedSourceMissing { .. } => None, - Problem::GeneratedValidationError { .. } => None, - Problem::ExtraFileStale { path, check_stale, .. } => { + VersionProblem::GeneratedSourceMissing { .. } => None, + VersionProblem::GeneratedValidationError { .. } => None, + VersionProblem::ExtraFileStale { path, check_stale, .. } => { Some(Fix::UpdateExtraFile { path, check_stale }) } - Problem::LatestLinkStale { api_ident, link, .. } - | Problem::LatestLinkMissing { api_ident, link } => { - Some(Fix::UpdateSymlink { api_ident, link }) - } - Problem::BlessedVersionShouldBeGitStub { local_file, git_stub } => { - Some(Fix::ConvertToGitStub { local_file, git_stub }) - } - Problem::GitStubShouldBeJson { local_file, blessed } => { + VersionProblem::BlessedVersionShouldBeGitStub { + local_file, + git_stub, + } => Some(Fix::ConvertToGitStub { local_file, git_stub }), + VersionProblem::GitStubShouldBeJson { local_file, blessed } => { Some(Fix::ConvertToJson { local_file, blessed }) } - Problem::BlessedVersionCorruptedLocal { + VersionProblem::BlessedVersionCorruptedLocal { local_file, blessed, git_stub, @@ -579,22 +635,17 @@ impl<'a> Problem<'a> { blessed, git_stub: git_stub.as_ref(), }), - Problem::DuplicateLocalFile { local_file } => { + VersionProblem::DuplicateLocalFile { local_file } => { Some(Fix::DeleteFiles { files: DisplayableVec(vec![ local_file.spec_file_name().clone(), ]), }) } - Problem::GitStubCommitStale { local_file, git_stub } => { + VersionProblem::GitStubCommitStale { local_file, git_stub } => { Some(Fix::UpdateGitStub { local_file, git_stub }) } - Problem::GitStubFirstCommitUnknown { .. } => None, - Problem::UnparseableLocalFile { unparseable_file } => { - Some(Fix::DeleteUnparseableFile { - path: unparseable_file.path.clone(), - }) - } + VersionProblem::GitStubFirstCommitUnknown { .. } => None, } } } @@ -1051,7 +1102,8 @@ fn symlink_file(target: &str, path: &Utf8Path) -> std::io::Result<()> { /// local spec files for a given API pub struct Resolved<'a> { notes: Vec, - non_version_problems: Vec<(ApiIdent, Option, Problem<'a>)>, + non_version_problems: + Vec<(ApiIdent, Option, NonVersionProblem<'a>)>, api_results: BTreeMap>, nexpected_documents: usize, } @@ -1101,7 +1153,7 @@ impl<'a> Resolved<'a> { let mut non_version_problems: Vec<( ApiIdent, Option, - Problem<'_>, + NonVersionProblem<'_>, )> = resolve_orphaned_local_specs(&supported_versions_by_api, local) .map(|spec_file_name| { let ident = spec_file_name.ident().clone(); @@ -1109,7 +1161,7 @@ impl<'a> Resolved<'a> { ( ident, version, - Problem::LocalSpecFileOrphaned { + NonVersionProblem::LocalSpecFileOrphaned { spec_file_name: spec_file_name.clone(), }, ) @@ -1143,7 +1195,7 @@ impl<'a> Resolved<'a> { Resolution { kind, problems: vec![ - Problem::GeneratedSourceMissing { + VersionProblem::GeneratedSourceMissing { api_ident: ident.clone(), }, ], @@ -1194,7 +1246,7 @@ impl<'a> Resolved<'a> { non_version_problems.push(( ident.clone(), None, - Problem::UnparseableLocalFile { + NonVersionProblem::UnparseableLocalFile { unparseable_file: unparseable.clone(), }, )); @@ -1218,7 +1270,9 @@ impl<'a> Resolved<'a> { self.notes.iter() } - pub fn general_problems(&self) -> impl Iterator> + '_ { + pub fn non_version_problems( + &self, + ) -> impl Iterator> + '_ { self.non_version_problems.iter().map(|(_, _, problem)| problem) } @@ -1230,24 +1284,26 @@ impl<'a> Resolved<'a> { self.api_results.get(ident).and_then(|v| v.by_version.get(version)) } - pub fn symlink_problem(&self, ident: &ApiIdent) -> Option<&Problem<'_>> { + pub fn symlink_problem( + &self, + ident: &ApiIdent, + ) -> Option<&NonVersionProblem<'_>> { self.api_results.get(ident).and_then(|v| v.symlink.as_ref()) } pub fn has_unfixable_problems(&self) -> bool { - self.general_problems().any(|p| !p.is_fixable()) + self.non_version_problems().any(|p| !p.is_fixable()) || self.api_results.values().any(|a| a.has_unfixable_problems()) } /// Returns an owned, ordered list of all problems as summaries. /// - /// Order: general (non-version-specific) problems first, then per-API - /// (sorted by ident), per-version (sorted by semver), then symlink - /// problems. + /// Order: non-version problems first, then per-API (sorted by ident), + /// per-version (sorted by semver), then symlink problems. pub fn problem_summaries(&self) -> Vec { let mut summaries = Vec::new(); - // General problems. + // Non-version problems. for (ident, version, problem) in &self.non_version_problems { summaries.push(ProblemSummary { api_ident: ident.clone(), @@ -1282,7 +1338,7 @@ impl<'a> Resolved<'a> { struct ApiResolved<'a> { by_version: BTreeMap>, - symlink: Option>, + symlink: Option>, } impl ApiResolved<'_> { @@ -1424,9 +1480,11 @@ fn resolve_api<'a>( version, Resolution { kind, - problems: vec![Problem::GeneratedSourceMissing { - api_ident: api.ident().clone(), - }], + problems: vec![ + VersionProblem::GeneratedSourceMissing { + api_ident: api.ident().clone(), + }, + ], }, ); }; @@ -1461,7 +1519,7 @@ fn resolve_api<'a>( if let Some((Some(spec_file_name), error)) = latest_first_commit_error && let Some(resolution) = by_version.get_mut(latest_version) { - resolution.add_problem(Problem::GitStubFirstCommitUnknown { + resolution.add_problem(VersionProblem::GitStubFirstCommitUnknown { spec_file_name, source: error, }); @@ -1559,7 +1617,7 @@ fn resolve_api<'a>( generated_version ); }); - Some(Problem::LatestLinkStale { + Some(NonVersionProblem::LatestLinkStale { api_ident: api.ident().clone(), link: blessed.versioned_spec_file_name(), found: latest_local, @@ -1568,7 +1626,7 @@ fn resolve_api<'a>( ResolutionKind::NewLocally => { // latest_generated is not blessed, so update // the symlink. - Some(Problem::LatestLinkStale { + Some(NonVersionProblem::LatestLinkStale { api_ident: api.ident().clone(), link: latest_generated, found: latest_local, @@ -1603,7 +1661,7 @@ fn resolve_api<'a>( generated_version ); }); - Some(Problem::LatestLinkMissing { + Some(NonVersionProblem::LatestLinkMissing { api_ident: api.ident().clone(), link: blessed.versioned_spec_file_name(), }) @@ -1611,7 +1669,7 @@ fn resolve_api<'a>( ResolutionKind::NewLocally => { // latest_generated is not blessed, so update // the symlink to the generated version. - Some(Problem::LatestLinkMissing { + Some(NonVersionProblem::LatestLinkMissing { api_ident: api.ident().clone(), link: latest_generated, }) @@ -1647,9 +1705,11 @@ fn resolve_api_lockstep<'a>( // didn't include this API's document). return BTreeMap::from([( version.clone(), - Resolution::new_lockstep(vec![Problem::GeneratedSourceMissing { - api_ident: api.ident().clone(), - }]), + Resolution::new_lockstep(vec![ + VersionProblem::GeneratedSourceMissing { + api_ident: api.ident().clone(), + }, + ]), )]); }; @@ -1691,9 +1751,11 @@ fn resolve_api_lockstep<'a>( match local { Some(local_file) if local_file.contents() == generated.contents() => (), Some(found) => { - problems.push(Problem::LockstepStale { found, generated }) + problems.push(VersionProblem::LockstepStale { found, generated }) + } + None => { + problems.push(VersionProblem::LockstepMissingLocal { generated }) } - None => problems.push(Problem::LockstepMissingLocal { generated }), }; BTreeMap::from([(version.clone(), Resolution::new_lockstep(problems))]) @@ -1773,13 +1835,13 @@ fn resolve_api_version_blessed<'a>( match api_compatible(blessed.value(), generated.value()) { Ok(issues) => { if !issues.is_empty() { - problems.push(Problem::BlessedVersionBroken { + problems.push(VersionProblem::BlessedVersionBroken { compatibility_issues: issues, }); } } Err(error) => { - problems.push(Problem::BlessedVersionCompareError { error }) + problems.push(VersionProblem::BlessedVersionCompareError { error }) } }; @@ -1793,7 +1855,7 @@ fn resolve_api_version_blessed<'a>( && problems.is_empty() && generated.contents() != blessed.contents() { - problems.push(Problem::BlessedLatestVersionBytewiseMismatch { + problems.push(VersionProblem::BlessedLatestVersionBytewiseMismatch { blessed, generated, }); @@ -1852,7 +1914,7 @@ fn resolve_api_version_blessed<'a>( // expensive because it may need to resolve a git revision to a commit // hash. let compute_storage_format = - |problems: &mut Vec>| -> VersionStorageFormat { + |problems: &mut Vec>| -> VersionStorageFormat { match git_stub { Some(r) => { match r.to_git_stub(&env.repo_root, merge_base, &env.vcs) { @@ -1861,12 +1923,14 @@ fn resolve_api_version_blessed<'a>( current, ), Err(error) => { - problems.push(Problem::GitStubFirstCommitUnknown { - spec_file_name: blessed - .versioned_spec_file_name() - .clone(), - source: error, - }); + problems.push( + VersionProblem::GitStubFirstCommitUnknown { + spec_file_name: blessed + .versioned_spec_file_name() + .clone(), + source: error, + }, + ); VersionStorageFormat::Error } } @@ -1880,13 +1944,13 @@ fn resolve_api_version_blessed<'a>( if use_git_stub_storage && !is_latest { match compute_storage_format(&mut problems) { VersionStorageFormat::GitStub(g) => { - problems.push(Problem::BlessedVersionMissingLocal { + problems.push(VersionProblem::BlessedVersionMissingLocal { blessed, git_stub: Some(g), }); } VersionStorageFormat::Json => { - problems.push(Problem::BlessedVersionMissingLocal { + problems.push(VersionProblem::BlessedVersionMissingLocal { blessed, git_stub: None, }); @@ -1899,7 +1963,7 @@ fn resolve_api_version_blessed<'a>( } } } else { - problems.push(Problem::BlessedVersionMissingLocal { + problems.push(VersionProblem::BlessedVersionMissingLocal { blessed, git_stub: None, }); @@ -1911,7 +1975,7 @@ fn resolve_api_version_blessed<'a>( // Report corrupted local files that need regeneration from blessed. for local_file in &corrupted { - problems.push(Problem::BlessedVersionCorruptedLocal { + problems.push(VersionProblem::BlessedVersionCorruptedLocal { local_file, blessed, git_stub: None, @@ -1927,14 +1991,18 @@ fn resolve_api_version_blessed<'a>( // case) for deletion. for local_file in matching { if local_file.spec_file_name().is_git_stub() { - problems.push(Problem::DuplicateLocalFile { local_file }); + problems.push(VersionProblem::DuplicateLocalFile { + local_file, + }); } } } else { let local_file = matching[0]; if local_file.spec_file_name().is_git_stub() { - problems - .push(Problem::GitStubShouldBeJson { local_file, blessed }); + problems.push(VersionProblem::GitStubShouldBeJson { + local_file, + blessed, + }); } } } else { @@ -1951,7 +2019,7 @@ fn resolve_api_version_blessed<'a>( None } }; - problems.push(Problem::BlessedVersionCorruptedLocal { + problems.push(VersionProblem::BlessedVersionCorruptedLocal { local_file, blessed, git_stub, @@ -1964,13 +2032,13 @@ fn resolve_api_version_blessed<'a>( let check_git_stub_staleness = |local_file: &'a LocalApiSpecFile, expected_git_stub: &GitStub, - problems: &mut Vec>| { + problems: &mut Vec>| { // Non-gitstub files (JSON) don't have a commit to check. let Some(local_commit) = local_file.git_stub_commit() else { return; }; if *local_commit != expected_git_stub.commit() { - problems.push(Problem::GitStubCommitStale { + problems.push(VersionProblem::GitStubCommitStale { local_file, git_stub: expected_git_stub.clone(), }); @@ -1993,8 +2061,9 @@ fn resolve_api_version_blessed<'a>( // have Git stub: this file is redundant. (VersionStorageFormat::GitStub(_), false) | (VersionStorageFormat::Json, true) => { - problems - .push(Problem::DuplicateLocalFile { local_file }); + problems.push(VersionProblem::DuplicateLocalFile { + local_file, + }); } // Format matches and is a Git stub: check for staleness. ( @@ -2018,14 +2087,16 @@ fn resolve_api_version_blessed<'a>( match (&storage_format, local_file.spec_file_name().is_git_stub()) { (VersionStorageFormat::GitStub(git_stub), false) => { // Should be Git stub but is JSON: convert to Git stub. - problems.push(Problem::BlessedVersionShouldBeGitStub { - local_file, - git_stub: git_stub.clone(), - }); + problems.push( + VersionProblem::BlessedVersionShouldBeGitStub { + local_file, + git_stub: git_stub.clone(), + }, + ); } (VersionStorageFormat::Json, true) => { // Should be JSON but is Git stub: convert to JSON. - problems.push(Problem::GitStubShouldBeJson { + problems.push(VersionProblem::GitStubShouldBeJson { local_file, blessed, }); @@ -2051,7 +2122,7 @@ fn resolve_api_version_blessed<'a>( // Report non-matching local files as extra. problems.extend(non_matching.into_iter().map(|s| { - Problem::BlessedVersionExtraLocalSpec { + VersionProblem::BlessedVersionExtraLocalSpec { spec_file_name: s .spec_file_name() .as_versioned() @@ -2084,10 +2155,11 @@ fn resolve_api_version_local<'a>( // There was no matching spec. if non_matching.is_empty() { // There were no non-matching specs, either. - problems.push(Problem::LocalVersionMissingLocal { generated }); + problems + .push(VersionProblem::LocalVersionMissingLocal { generated }); } else { // There were non-matching specs. This is your basic "stale" case. - problems.push(Problem::LocalVersionStale { + problems.push(VersionProblem::LocalVersionStale { spec_files: non_matching, generated, }); @@ -2106,7 +2178,7 @@ fn resolve_api_version_local<'a>( }) .collect(), ); - problems.push(Problem::LocalVersionExtra { spec_file_names }); + problems.push(VersionProblem::LocalVersionExtra { spec_file_names }); } Resolution::new_new_locally(problems) @@ -2118,7 +2190,7 @@ fn validate_generated( validation: Option<&DynValidationFn>, version: ApiVersion<'_>, generated: &GeneratedApiSpecFile, - problems: &mut Vec>, + problems: &mut Vec>, ) { match validate( env, @@ -2129,7 +2201,7 @@ fn validate_generated( generated, ) { Err(source) => { - problems.push(Problem::GeneratedValidationError { + problems.push(VersionProblem::GeneratedValidationError { api_ident: api.ident().clone(), version: version.version.clone(), source, @@ -2140,7 +2212,7 @@ fn validate_generated( match status { CheckStatus::Fresh => (), CheckStatus::Stale(check_stale) => { - problems.push(Problem::ExtraFileStale { + problems.push(VersionProblem::ExtraFileStale { api_ident: api.ident().clone(), path, check_stale, From de6001f086488afbbbaa78792ee756c28eeba476 Mon Sep 17 00:00:00 2001 From: Rain Date: Thu, 14 May 2026 23:48:50 -0700 Subject: [PATCH 2/3] simplifications Created using spr 1.3.6-beta.1 --- crates/dropshot-api-manager/src/output.rs | 141 +++++++++----------- crates/dropshot-api-manager/src/resolved.rs | 17 ++- 2 files changed, 74 insertions(+), 84 deletions(-) diff --git a/crates/dropshot-api-manager/src/output.rs b/crates/dropshot-api-manager/src/output.rs index 992704d..a118b4c 100644 --- a/crates/dropshot-api-manager/src/output.rs +++ b/crates/dropshot-api-manager/src/output.rs @@ -5,7 +5,8 @@ use crate::{ apis::{ManagedApi, ManagedApis}, environment::{ErrorAccumulator, ResolvedEnv}, resolved::{ - NonVersionProblem, Resolution, ResolutionKind, Resolved, VersionProblem, + Fix, NonVersionProblem, Resolution, ResolutionKind, Resolved, + VersionProblem, }, validation::CheckStale, }; @@ -516,26 +517,12 @@ where T: IntoIterator>, { for p in problems.into_iter() { - let subheader_width = HEADER_WIDTH + 4; - let first_indent = format!( - "{:>subheader_width$}: ", - if p.is_fixable() { - "problem".style(styles.warning_header) - } else { - "error".style(styles.failure_header) - } - ); - let more_indent = " ".repeat(subheader_width + 2); - writeln!( - writer, - "{}", - textwrap::fill( - &InlineErrorChain::new(&p).to_string(), - textwrap::Options::with_termwidth() - .initial_indent(&first_indent) - .subsequent_indent(&more_indent) - ) - )?; + write_problem_header(writer, &p, p.is_fixable(), styles)?; + + // Body indent used by the nested BlessedVersionBroken issue list. + // Mirrors the continuation indent inside the problem header so + // wrapped issue lines line up under the header text. + let more_indent = " ".repeat(HEADER_WIDTH + 4 + 2); // For BlessedVersionBroken, print each item separately, along with a // diff between blessed and generated versions. @@ -612,24 +599,7 @@ where continue; }; - let first_indent = format!( - "{:>subheader_width$}: ", - "fix".style(styles.warning_header) - ); - let fix_str = fix.to_string(); - let steps = fix_str.trim_end().split("\n"); - for s in steps { - writeln!( - writer, - "{}", - textwrap::fill( - &format!("will {}", s), - textwrap::Options::with_termwidth() - .initial_indent(&first_indent) - .subsequent_indent(&more_indent) - ) - )?; - } + write_fix_summary(writer, &fix, styles)?; // When possible, print a useful diff of changes. let do_diff = match p { @@ -691,9 +661,10 @@ where Ok(()) } -/// Print a formatted list of [`NonVersionProblem`]s (orphaned files, -/// unparseable files, and "latest" symlink issues) to `writer`. None of -/// these variants have associated diffs, so this is just header + fix. +/// Print a formatted list of [`NonVersionProblem`]s to `writer`. +/// +/// None of these variants have associated diffs, so this is just a header + +/// fix. pub fn display_non_version_problems<'a, T>( writer: &mut dyn io::Write, problems: T, @@ -703,48 +674,68 @@ where T: IntoIterator>, { for p in problems.into_iter() { - let subheader_width = HEADER_WIDTH + 4; - let first_indent = format!( - "{:>subheader_width$}: ", - if p.is_fixable() { - "problem".style(styles.warning_header) - } else { - "error".style(styles.failure_header) - } - ); - let more_indent = " ".repeat(subheader_width + 2); + write_problem_header(writer, &p, p.is_fixable(), styles)?; + if let Some(fix) = p.fix() { + write_fix_summary(writer, &fix, styles)?; + } + } + Ok(()) +} + +/// Write the `problem:` / `error:` header line for a single problem, wrapping +/// the error chain to the terminal width. +fn write_problem_header( + writer: &mut dyn io::Write, + error: &dyn std::error::Error, + is_fixable: bool, + styles: &Styles, +) -> io::Result<()> { + let subheader_width = HEADER_WIDTH + 4; + let first_indent = format!( + "{:>subheader_width$}: ", + if is_fixable { + "problem".style(styles.warning_header) + } else { + "error".style(styles.failure_header) + } + ); + let more_indent = " ".repeat(subheader_width + 2); + writeln!( + writer, + "{}", + textwrap::fill( + &InlineErrorChain::new(error).to_string(), + textwrap::Options::with_termwidth() + .initial_indent(&first_indent) + .subsequent_indent(&more_indent) + ) + ) +} + +/// Write the `fix:` line(s) for a single fix, splitting multi-step fixes into +/// separate `will ...` lines that share the column structure of +/// [`write_problem_header`]. +fn write_fix_summary( + writer: &mut dyn io::Write, + fix: &Fix<'_>, + styles: &Styles, +) -> io::Result<()> { + let subheader_width = HEADER_WIDTH + 4; + let first_indent = + format!("{:>subheader_width$}: ", "fix".style(styles.warning_header)); + let more_indent = " ".repeat(subheader_width + 2); + let fix_str = fix.to_string(); + for s in fix_str.trim_end().split("\n") { writeln!( writer, "{}", textwrap::fill( - &InlineErrorChain::new(&p).to_string(), + &format!("will {}", s), textwrap::Options::with_termwidth() .initial_indent(&first_indent) .subsequent_indent(&more_indent) ) )?; - - let Some(fix) = p.fix() else { - continue; - }; - - let first_indent = format!( - "{:>subheader_width$}: ", - "fix".style(styles.warning_header) - ); - let fix_str = fix.to_string(); - for s in fix_str.trim_end().split("\n") { - writeln!( - writer, - "{}", - textwrap::fill( - &format!("will {}", s), - textwrap::Options::with_termwidth() - .initial_indent(&first_indent) - .subsequent_indent(&more_indent) - ) - )?; - } } Ok(()) } diff --git a/crates/dropshot-api-manager/src/resolved.rs b/crates/dropshot-api-manager/src/resolved.rs index b9e3865..a0e232e 100644 --- a/crates/dropshot-api-manager/src/resolved.rs +++ b/crates/dropshot-api-manager/src/resolved.rs @@ -210,13 +210,14 @@ impl ProblemSummary { } } -/// Describes a problem with the blessed spec(s), generated spec(s), and local -/// spec files for an API that is *not* tied to a specific supported version. +/// Describes a problem resolving the blessed spec(s), generated spec(s), and +/// local spec files for an API that is *not* tied to a specific supported +/// version. /// /// Per-(api, version) problems live in [`VersionProblem`] instead. Splitting -/// the two means callers can prove at compile time that, e.g., the symlink -/// problem slot can never hold a `BlessedVersionBroken`, and rendering code -/// that needs version-specific context can require it unconditionally. +/// the two means that we can establish at compile time that, e.g., symlinks can +/// never have `BlessedVersionBroken`, and rendering code that needs +/// version-specific context can require it unconditionally. #[derive(Debug, Error)] pub enum NonVersionProblem<'a> { #[error( @@ -259,10 +260,8 @@ pub enum NonVersionProblem<'a> { /// Describes a problem with a specific supported (api, version) pair. /// -/// Companion to [`NonVersionProblem`], which holds problems that aren't tied to a -/// specific version. The split lets rendering code that needs per-(api, -/// version) context (e.g., for compatibility-issue dedup) require that -/// context at the type level. +/// Companion to [`NonVersionProblem`], which holds problems that aren't tied to +/// a specific version. #[derive(Debug, Error)] pub enum VersionProblem<'a> { #[error( From 847d8b1aedb968d8f801fa780441511ea572f364 Mon Sep 17 00:00:00 2001 From: Rain Date: Fri, 15 May 2026 00:12:53 -0700 Subject: [PATCH 3/3] more simplifications Created using spr 1.3.6-beta.1 --- crates/dropshot-api-manager/src/cmd/debug.rs | 2 +- .../dropshot-api-manager/src/cmd/generate.rs | 33 ++++++++-------- crates/dropshot-api-manager/src/output.rs | 14 +++---- crates/dropshot-api-manager/src/resolved.rs | 39 +++++++++++++------ 4 files changed, 52 insertions(+), 36 deletions(-) diff --git a/crates/dropshot-api-manager/src/cmd/debug.rs b/crates/dropshot-api-manager/src/cmd/debug.rs index f611d83..054b980 100644 --- a/crates/dropshot-api-manager/src/cmd/debug.rs +++ b/crates/dropshot-api-manager/src/cmd/debug.rs @@ -59,7 +59,7 @@ pub(crate) fn debug_impl( for note in resolved.notes() { println!("NOTE: {}", note); } - for problem in resolved.non_version_problems() { + for problem in resolved.orphaned_and_unparseable() { println!("PROBLEM: {}", problem); } diff --git a/crates/dropshot-api-manager/src/cmd/generate.rs b/crates/dropshot-api-manager/src/cmd/generate.rs index ff2e6b0..1c0b2e4 100644 --- a/crates/dropshot-api-manager/src/cmd/generate.rs +++ b/crates/dropshot-api-manager/src/cmd/generate.rs @@ -11,7 +11,7 @@ use crate::{ headers::{self, *}, plural, }, - resolved::{Fix, NonVersionProblem, Resolved, VersionProblem}, + resolved::{Fix, Resolved}, }; use anyhow::{Result, anyhow, bail}; use owo_colors::OwoColorize; @@ -119,8 +119,7 @@ fn generate_impl_inner( checked above" ); - let problems: Vec<_> = resolution.problems().collect(); - if problems.is_empty() { + if !resolution.has_problems() { writeln!( writer, "{:>HEADER_WIDTH$} {}", @@ -139,7 +138,7 @@ fn generate_impl_inner( apply_fixes( writer, env, - problems.into_iter().map(expect_version_fix), + resolution.problems().map(|p| expect_fix(p.fix())), styles, &mut num_updated, &mut num_errors, @@ -158,7 +157,7 @@ fn generate_impl_inner( apply_fixes( writer, env, - std::iter::once(expect_non_version_fix(symlink_problem)), + std::iter::once(expect_fix(symlink_problem.fix())), styles, &mut num_updated, &mut num_errors, @@ -177,7 +176,7 @@ fn generate_impl_inner( apply_fixes( writer, env, - resolved.non_version_problems().map(expect_non_version_fix), + resolved.orphaned_and_unparseable().map(|p| expect_fix(p.fix())), styles, &mut num_updated, &mut num_errors, @@ -217,11 +216,11 @@ fn generate_impl_inner( display_load_problems(writer, &errors, styles)?; let resolved = Resolved::new(env, apis, &blessed, &generated, &local_files_recheck); - let non_version_problems: Vec<_> = - resolved.non_version_problems().collect(); - nproblems += non_version_problems.len(); - if !non_version_problems.is_empty() { - display_non_version_problems(writer, non_version_problems, styles)?; + let orphaned_and_unparseable: Vec<_> = + resolved.orphaned_and_unparseable().collect(); + nproblems += orphaned_and_unparseable.len(); + if !orphaned_and_unparseable.is_empty() { + display_non_version_problems(writer, orphaned_and_unparseable, styles)?; } for api in apis.iter_apis() { let ident = api.ident(); @@ -314,12 +313,12 @@ fn print_final_status( Ok(()) } -fn expect_version_fix<'a>(p: &'a VersionProblem<'a>) -> Fix<'a> { - p.fix().expect("attempting to fix unfixable problem") -} - -fn expect_non_version_fix<'a>(p: &'a NonVersionProblem<'a>) -> Fix<'a> { - p.fix().expect("attempting to fix unfixable problem") +/// Unwrap a `fix()` result at an `apply_fixes` call site. +/// +/// Precondition: [`Resolved::has_unfixable_problems`] returned false, so +/// every reachable problem must produce a fix. +fn expect_fix<'a>(fix: Option>) -> Fix<'a> { + fix.expect("problem is fixable per has_unfixable_problems guard") } fn apply_fixes<'a>( diff --git a/crates/dropshot-api-manager/src/output.rs b/crates/dropshot-api-manager/src/output.rs index a118b4c..b87d04d 100644 --- a/crates/dropshot-api-manager/src/output.rs +++ b/crates/dropshot-api-manager/src/output.rs @@ -362,9 +362,9 @@ pub fn display_resolution( } // Print problems not associated with any supported version, if any. - let non_version_problems: Vec<_> = - resolved.non_version_problems().collect(); - num_non_version_problems += if !non_version_problems.is_empty() { + let orphaned_and_unparseable: Vec<_> = + resolved.orphaned_and_unparseable().collect(); + num_non_version_problems += if !orphaned_and_unparseable.is_empty() { writeln!( writer, "\n{:>HEADER_WIDTH$} problems not associated with a specific \ @@ -375,9 +375,9 @@ pub fn display_resolution( let (fixable, unfixable): ( Vec<&NonVersionProblem>, Vec<&NonVersionProblem>, - ) = non_version_problems.iter().partition(|p| p.is_fixable()); + ) = orphaned_and_unparseable.iter().partition(|p| p.is_fixable()); num_failed += unfixable.len(); - display_non_version_problems(writer, non_version_problems, styles)?; + display_non_version_problems(writer, orphaned_and_unparseable, styles)?; fixable.len() } else { 0 @@ -517,7 +517,7 @@ where T: IntoIterator>, { for p in problems.into_iter() { - write_problem_header(writer, &p, p.is_fixable(), styles)?; + write_problem_header(writer, p, p.is_fixable(), styles)?; // Body indent used by the nested BlessedVersionBroken issue list. // Mirrors the continuation indent inside the problem header so @@ -674,7 +674,7 @@ where T: IntoIterator>, { for p in problems.into_iter() { - write_problem_header(writer, &p, p.is_fixable(), styles)?; + write_problem_header(writer, p, p.is_fixable(), styles)?; if let Some(fix) = p.fix() { write_fix_summary(writer, &fix, styles)?; } diff --git a/crates/dropshot-api-manager/src/resolved.rs b/crates/dropshot-api-manager/src/resolved.rs index a0e232e..6039f5d 100644 --- a/crates/dropshot-api-manager/src/resolved.rs +++ b/crates/dropshot-api-manager/src/resolved.rs @@ -1101,7 +1101,12 @@ fn symlink_file(target: &str, path: &Utf8Path) -> std::io::Result<()> { /// local spec files for a given API pub struct Resolved<'a> { notes: Vec, - non_version_problems: + /// Non-version problems that aren't attached to a supported (api, version) + /// pair: local files for unsupported versions and unparseable local + /// files. The "latest" symlink problems are *also* non-version problems, + /// but they live on [`ApiResolved::symlink`] and are reached via + /// [`Resolved::symlink_problem`]. + orphaned_and_unparseable: Vec<(ApiIdent, Option, NonVersionProblem<'a>)>, api_results: BTreeMap>, nexpected_documents: usize, @@ -1149,7 +1154,7 @@ impl<'a> Resolved<'a> { // Get the other easy case out of the way: if there are any local spec // files for APIs or API versions that aren't supported any more, that's // a (fixable) problem. - let mut non_version_problems: Vec<( + let mut orphaned_and_unparseable: Vec<( ApiIdent, Option, NonVersionProblem<'_>, @@ -1242,7 +1247,7 @@ impl<'a> Resolved<'a> { for unparseable in api_files.unparseable_files() { // Only report if no fix will overwrite this path. if !paths_written.contains(&unparseable.path) { - non_version_problems.push(( + orphaned_and_unparseable.push(( ident.clone(), None, NonVersionProblem::UnparseableLocalFile { @@ -1255,7 +1260,7 @@ impl<'a> Resolved<'a> { Resolved { notes, - non_version_problems, + orphaned_and_unparseable, api_results, nexpected_documents, } @@ -1269,10 +1274,17 @@ impl<'a> Resolved<'a> { self.notes.iter() } - pub fn non_version_problems( + /// Iterate over non-version problems that aren't attached to a specific + /// supported (api, version) pair: local files for unsupported versions + /// and unparseable local files. + /// + /// Does *not* include "latest" symlink problems, which are also + /// [`NonVersionProblem`]s but are reached via + /// [`Resolved::symlink_problem`]. + pub fn orphaned_and_unparseable( &self, ) -> impl Iterator> + '_ { - self.non_version_problems.iter().map(|(_, _, problem)| problem) + self.orphaned_and_unparseable.iter().map(|(_, _, problem)| problem) } pub fn resolution_for_api_version( @@ -1283,6 +1295,11 @@ impl<'a> Resolved<'a> { self.api_results.get(ident).and_then(|v| v.by_version.get(version)) } + /// Returns the "latest" symlink problem for an API, if any. + /// + /// Symlink problems are [`NonVersionProblem`]s but live separately from + /// [`Resolved::orphaned_and_unparseable`] because they're scoped to an + /// API rather than a (file, version) pair. pub fn symlink_problem( &self, ident: &ApiIdent, @@ -1291,19 +1308,19 @@ impl<'a> Resolved<'a> { } pub fn has_unfixable_problems(&self) -> bool { - self.non_version_problems().any(|p| !p.is_fixable()) + self.orphaned_and_unparseable().any(|p| !p.is_fixable()) || self.api_results.values().any(|a| a.has_unfixable_problems()) } /// Returns an owned, ordered list of all problems as summaries. /// - /// Order: non-version problems first, then per-API (sorted by ident), - /// per-version (sorted by semver), then symlink problems. + /// Order: orphaned/unparseable problems first, then per-API (sorted by + /// ident), per-version (sorted by semver), then symlink problems. pub fn problem_summaries(&self) -> Vec { let mut summaries = Vec::new(); - // Non-version problems. - for (ident, version, problem) in &self.non_version_problems { + // Orphaned and unparseable problems. + for (ident, version, problem) in &self.orphaned_and_unparseable { summaries.push(ProblemSummary { api_ident: ident.clone(), version: version.clone(),