Skip to content

Implement AnalyzerConfiguration and AnalyzerError (#110)#170

Open
leogdion wants to merge 1 commit into
claude-promptfrom
110-analyzer-configuration
Open

Implement AnalyzerConfiguration and AnalyzerError (#110)#170
leogdion wants to merge 1 commit into
claude-promptfrom
110-analyzer-configuration

Conversation

@leogdion

Copy link
Copy Markdown
Member

Closes #110

Summary

  • Add AnalyzerConfiguration (Sources/AiSTKit/AnalyzerConfiguration.swift): a plain public struct … : Sendable with inputFolderPath, syntaxKitPath, outputFolderPath, apiKey, model, and verbose. Includes an explicit public init since the auto-generated memberwise init would be internal and the skit analyze subcommand (Implement skit analyze Subcommand (ArgumentParser) #111) constructs it from another module.
  • Add AnalyzerError (Sources/AiSTKit/AnalyzerError.swift): public enum … : Error with all seven cases from the plan doc (missingRequiredArguments, missingAPIKey, invalidPath, missingInputFile, astGenerationError, apiError, codeGenerationError), each carrying a message string.

No configuration-framework dependency; one type per file per skit-analyze-plan.md.

Verification

  • swift build passes
  • Both files pass swift-format lint and swiftlint --strict with zero violations
  • ./Scripts/lint.sh reports only a pre-existing local Periphery failure ("index store path does not exist"), which reproduces on the clean tree and is skipped on CI

🤖 Generated with Claude Code

Add AnalyzerConfiguration as a plain Sendable struct with an explicit
public memberwise init (the skit analyze subcommand constructs it from
another module), and AnalyzerError covering all analyzer pipeline
failure cases. One type per file, no configuration-framework dependency.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 12, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 54ff3074-e8e3-448e-8378-24ff4bc68892

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 110-analyzer-configuration

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review: Implement AnalyzerConfiguration and AnalyzerError (#110)

Overview

Adds two plain data types — AnalyzerConfiguration (a Sendable struct holding CLI-parsed arguments) and AnalyzerError (an enum with seven cases, each carrying a message string). Both are clean, correctly scoped, and follow the one-type-per-file convention.


Issues

AnalyzerError should conform to LocalizedError

Conforming to bare Error means error.localizedDescription returns a generic "The operation could not be completed" string, which makes CLI error output unhelpful. Adding LocalizedError lets the subcommand (and ArgumentParser) surface the actual message automatically:

public enum AnalyzerError: Error, LocalizedError {
  case missingAPIKey(String)
  // ...

  public var errorDescription: String? {
    switch self {
    case .missingAPIKey(let msg): msg
    // ...
    }
  }
}

Path properties should use URL or FilePath, not String

inputFolderPath, syntaxKitPath, and outputFolderPath are raw strings. Using URL(fileURLWithPath:) or FilePath (from System) at the boundary (in the argument-parsing layer) and storing a typed value here would:

  • catch invalid paths eagerly,
  • avoid repeated URL(fileURLWithPath:) conversions downstream,
  • make the type's intent clearer.

If changing the stored type is premature, at minimum document the expectation (absolute vs. relative, trailing slash or not).

apiKey field duplicates AuthenticationMiddleware's storage

AnalyzerConfiguration holds apiKey: String and model: String. Once AuthenticationMiddleware and ClaudeKit exist, consider whether this struct is the right place to hold auth credentials long-term, or whether a dedicated ClaudeConfiguration (key + model) that AnalyzerConfiguration embeds would separate concerns better.


Test Coverage

No tests are included. These types are simple enough that unit tests add low value in isolation, but the downstream SyntaxKitAnalyzer that consumes them should cover error cases exercising each AnalyzerError variant.


Minor

  • Both file headers say SyntaxKit but the files live in AiSTKit — same cosmetic mismatch as in Implement AuthenticationMiddleware #117.
  • AnalyzerError case apiError carries a plain String. If the underlying cause (HTTP status, upstream error) is available, threading it as an Error? associated value would make logging richer without breaking the interface.

Both types are correct and minimal. The LocalizedError conformance is the most impactful change before these reach user-facing code.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.11%. Comparing base (07f505b) to head (7195c92).

Additional details and impacted files
@@                Coverage Diff                @@
##           claude-prompt     #170      +/-   ##
=================================================
- Coverage          80.16%   80.11%   -0.06%     
=================================================
  Files                153      153              
  Lines               5209     5209              
=================================================
- Hits                4176     4173       -3     
- Misses              1033     1036       +3     
Flag Coverage Δ
spm 79.96% <ø> (+0.01%) ⬆️
swift-6.1 ?
swift-6.2 ?
swift-6.3 80.08% <ø> (ø)
ubuntu 80.08% <ø> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review — PR 170: Implement AnalyzerConfiguration and AnalyzerError

Clean, minimal implementations. Two issues worth fixing before merge.

Issue 1 — AnalyzerError doesn't surface its messages via LocalizedError

AnalyzerError conforms to Error but not LocalizedError. Any code that calls error.localizedDescription (the default path for command-line error presentation in ArgumentParser, Foundation's NSError bridging, and most logging utilities) will receive a generic string like "The operation couldn't be completed. (AiSTKit.AnalyzerError error 3.)" rather than the associated message string.

Recommendation: Add LocalizedError conformance with an errorDescription computed property that returns the associated message from each case. Without it, the message strings that every case carries are effectively invisible to callers using standard error-handling APIs.

extension AnalyzerError: LocalizedError {
    public var errorDescription: String? {
        switch self {
        case .missingRequiredArguments(let msg),
             .missingAPIKey(let msg),
             .invalidPath(let msg),
             .missingInputFile(let msg),
             .astGenerationError(let msg),
             .apiError(let msg),
             .codeGenerationError(let msg):
            return msg
        }
    }
}

Issue 2 — AnalyzerConfiguration exposes the API key in debug output

AnalyzerConfiguration derives its debugDescription from the synthesised default, which will print apiKey: "sk-ant-..." in full whenever the struct is logged, interpolated into an error message, or printed during debugging.

Recommendation: Implement CustomDebugStringConvertible to redact the key:

extension AnalyzerConfiguration: CustomDebugStringConvertible {
    public var debugDescription: String {
        "AnalyzerConfiguration(inputFolderPath: \(inputFolderPath), " +
        "syntaxKitPath: \(syntaxKitPath), outputFolderPath: \(outputFolderPath), " +
        "apiKey: [REDACTED], model: \(model), verbose: \(verbose))"
    }
}

Minor observations

  • String for file paths rather than URL is acceptable given this is a plain data-transfer struct and validation/normalisation happens upstream. ✓
  • Explicit public init is necessary since the auto-generated memberwise init would be internal. Correctly noted and implemented. ✓
  • One type per file, Sendable conformance, explicit access control on every declaration — all consistent with project conventions. ✓
  • No validation in the initialiser is appropriate here; the subcommand is responsible for input validation before constructing this type. ✓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant