Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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

Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion crates/dropshot-api-manager/src/cmd/debug.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.orphaned_and_unparseable() {
println!("PROBLEM: {}", problem);
}

Expand Down
57 changes: 29 additions & 28 deletions crates/dropshot-api-manager/src/cmd/generate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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, Resolved},
};
use anyhow::{Result, anyhow, bail};
use owo_colors::OwoColorize;
Expand Down Expand Up @@ -118,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$} {}",
Expand All @@ -135,10 +135,10 @@ fn generate_impl_inner(
display_api_spec_version(api, version, styles, resolution),
)?;

fix_problems(
apply_fixes(
writer,
env,
problems,
resolution.problems().map(|p| expect_fix(p.fix())),
styles,
&mut num_updated,
&mut num_errors,
Expand All @@ -154,10 +154,10 @@ fn generate_impl_inner(
ident.style(styles.filename),
)?;

fix_problems(
apply_fixes(
writer,
env,
std::iter::once(symlink_problem),
std::iter::once(expect_fix(symlink_problem.fix())),
styles,
&mut num_updated,
&mut num_errors,
Expand All @@ -173,11 +173,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.orphaned_and_unparseable().map(|p| expect_fix(p.fix())),
styles,
&mut num_updated,
&mut num_errors,
Expand Down Expand Up @@ -217,10 +216,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 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();
Expand All @@ -237,7 +237,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)?;
}
}

Expand All @@ -248,9 +248,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,
)?;
Expand Down Expand Up @@ -314,21 +313,23 @@ fn print_final_status(
Ok(())
}

fn fix_problems<'a, T>(
/// 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<'a> {
fix.expect("problem is fixable per has_unfixable_problems guard")
}

fn apply_fixes<'a>(
writer: &mut dyn io::Write,
env: &ResolvedEnv,
problems: T,
fixes: impl IntoIterator<Item = Fix<'a>>,
styles: &Styles,
num_updated: &mut usize,
num_errors: &mut usize,
) -> io::Result<()>
where
T: IntoIterator<Item = &'a Problem<'a>>,
{
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;
Expand Down
Loading
Loading