Code Cleanup and Simplification

[ksylvan]

Code Cleanup and Simplification

Summary

This pull request focuses on code refactoring and cleanup across several internal packages. The primary changes include simplifying redundant error formatting in the CLI module, streamlining model normalization logic in the core chatter module, and removing an unused parameter from the vendor management system.

Files Changed

internal/cli/flags.go

• Change: Removed redundant fmt.Sprintf calls within fmt.Errorf.
• Reason: fmt.Errorf natively supports format verbs. Wrapping a fmt.Sprintf call inside fmt.Errorf("%s", ...) is unnecessary, adds extra allocations, and reduces readability.

internal/core/chatter.go

• Change: Simplified the assignment of the model name in the Send method.
• Reason: The previous logic contained an if/else block where both branches assigned o.model to opts.Model. This has been collapsed into a single, direct assignment.

internal/plugins/ai/vendors.go

• Change: Removed the unused variadic parameter vendors ...Vendor from the Clear method signature.
• Reason: The method resets the internal maps and slices entirely; the parameters were not being utilized in the function body.

Code Changes

Redundant Formatting Cleanup

In internal/cli/flags.go, multiple instances of nested formatting were simplified:

// Before
return fmt.Errorf("%s", fmt.Sprintf(i18n.T("cannot_convert_string"), str, targetField.Kind()))

// After
return fmt.Errorf(i18n.T("cannot_convert_string"), str, targetField.Kind())

Logic Simplification

In internal/core/chatter.go, the redundant conditional check was removed:

// Before
if opts.Model == "" {
    opts.Model = o.model
} else {
    opts.Model = o.model
}

// After
opts.Model = o.model

API Cleanup

In internal/plugins/ai/vendors.go, the method signature was updated to reflect its actual behavior:

// Before
func (o *VendorsManager) Clear(vendors ...Vendor) { ... }

// After
func (o *VendorsManager) Clear() { ... }

Reason for Changes

The main driver for these changes is technical debt reduction.

1. Efficiency: Removing nested fmt.Sprintf calls reduces the number of string allocations and function calls during error handling.
2. Readability: Simplifying the logic in chatter.go makes the intent of the code (always using the normalized model name) much clearer.
3. Correctness: The Clear method signature was misleading as it suggested it could clear specific vendors, while it actually performed a full reset.

Impact of Changes

• Performance: Negligible but positive impact on memory usage during error states.
• Maintainability: The codebase is cleaner and follows Go idioms more closely.
• Breaking Change: The change to VendorsManager.Clear() is a breaking change for any internal callers that were passing arguments to this function. However, since the arguments were ignored, the logic remains identical.

Test Plan

1. Unit Tests: Run existing tests for internal/cli and internal/core to ensure error messages and model assignments still behave as expected.
2. Manual Verification: Trigger a CLI error (e.g., invalid config path) to verify that the i18n string interpolation still functions correctly.
3. Compilation Check: Ensure all internal callers of VendorsManager.Clear() are updated (if any existed).

Additional Notes

These changes produce no functional change and are zero risk, however, they do improve some of the error message
outputs.

For example, introducing a syntax error in ~/.config/fabric/config.yml and then running this:

$ fabric  
error parsing config file: %!w(*errors.errorString=&{yaml: line 3: could not find expected ':'})

After the fixes,

$ fabric                 
error parsing config file: yaml: line 3: could not find expected ':'

[Copilot]

Pull request overview

This PR performs code cleanup and simplification across three internal packages, focusing on removing redundant code patterns and improving error message formatting. The changes eliminate unnecessary fmt.Sprintf wrapping within fmt.Errorf calls, simplify redundant conditional logic, and remove an unused method parameter.

• Removed nested fmt.Sprintf calls in error formatting throughout the CLI flags module
• Simplified redundant if/else block in the chatter module that assigned the same value in both branches
• Removed unused variadic parameter from the VendorsManager.Clear() method

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
internal/cli/flags.go	Removed redundant fmt.Sprintf wrapper from 13 error formatting statements, allowing fmt.Errorf to directly handle i18n format strings
internal/core/chatter.go	Simplified redundant conditional logic that assigned o.model to opts.Model in both if and else branches
internal/plugins/ai/vendors.go	Removed unused vendors ...Vendor parameter from Clear() method signature that wasn't referenced in the method body

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.