From 66025d516c1de8594bdb25d3a6f3c3ddca30d8e7 Mon Sep 17 00:00:00 2001 From: Kayvan Sylvan Date: Mon, 15 Dec 2025 22:08:03 -0800 Subject: [PATCH] refactor: replace interface{} with any across codebase - Part 1 of incorporating `modernize` tool into Fabric. - Replace `interface{}` with `any` in slice type declarations - Update map types from `map[string]interface{}` to `map[string]any` - Change variadic function parameters to use `...any` instead of `...interface{}` - Modernize JSON unmarshaling variables to `any` for consistency - Update struct fields and method signatures to prefer `any` alias - Ensure all type assertions and conversions use `any` throughout codebase - Add PR guidelines in docs to encourage focused, reviewable changes --- cmd/code_helper/code.go | 6 ++-- cmd/generate_changelog/incoming/1874.txt | 8 +++++ .../internal/github/client.go | 2 +- docs/CONTRIBUTING.md | 23 ++++++++++++ internal/log/log.go | 4 +-- internal/plugins/ai/anthropic/oauth_test.go | 12 +++---- internal/plugins/ai/lmstudio/lmstudio.go | 36 +++++++++---------- internal/plugins/ai/ollama/ollama.go | 2 +- .../plugins/ai/openai/openai_image_test.go | 10 +++--- internal/plugins/db/fsdb/storage.go | 4 +-- .../template/extension_executor_test.go | 12 +++---- .../plugins/template/extension_registry.go | 10 +++--- internal/plugins/template/template.go | 2 +- internal/server/models.go | 2 +- 14 files changed, 82 insertions(+), 51 deletions(-) create mode 100644 cmd/generate_changelog/incoming/1874.txt diff --git a/cmd/code_helper/code.go b/cmd/code_helper/code.go index 4794feda..2f165fff 100644 --- a/cmd/code_helper/code.go +++ b/cmd/code_helper/code.go @@ -109,11 +109,11 @@ func ScanDirectory(rootDir string, maxDepth int, instructions string, ignoreList } // Create final data structure - var data []interface{} + var data []any data = append(data, rootItem) // Add report - reportItem := map[string]interface{}{ + reportItem := map[string]any{ "type": "report", "directories": dirCount, "files": fileCount, @@ -121,7 +121,7 @@ func ScanDirectory(rootDir string, maxDepth int, instructions string, ignoreList data = append(data, reportItem) // Add instructions - instructionsItem := map[string]interface{}{ + instructionsItem := map[string]any{ "type": "instructions", "name": "code_change_instructions", "details": instructions, diff --git a/cmd/generate_changelog/incoming/1874.txt b/cmd/generate_changelog/incoming/1874.txt new file mode 100644 index 00000000..ae25f895 --- /dev/null +++ b/cmd/generate_changelog/incoming/1874.txt @@ -0,0 +1,8 @@ +### PR [#1874](https://github.com/danielmiessler/Fabric/pull/1874) by [ksylvan](https://github.com/ksylvan): refactor: replace interface{} with any across codebase + +- Part 1 of dealing with #1873 as pointed out by @philoserf +- Replace `interface{}` with `any` in slice type declarations throughout the codebase +- Update map types from `map[string]interface{}` to `map[string]any` for modern Go standards +- Change variadic function parameters to use `...any` instead of `...interface{}` +- Modernize JSON unmarshaling variables to use `any` for consistency +- Update struct fields and method signatures to prefer the `any` alias over legacy interface syntax diff --git a/cmd/generate_changelog/internal/github/client.go b/cmd/generate_changelog/internal/github/client.go index 1cba38bc..ab61504a 100644 --- a/cmd/generate_changelog/internal/github/client.go +++ b/cmd/generate_changelog/internal/github/client.go @@ -333,7 +333,7 @@ func (c *Client) FetchAllMergedPRsGraphQL(since time.Time) ([]*PR, error) { for { // Prepare variables - variables := map[string]interface{}{ + variables := map[string]any{ "owner": graphql.String(c.owner), "repo": graphql.String(c.repo), "after": (*graphql.String)(after), diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 6137b26a..4d67a9d9 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -51,6 +51,29 @@ docs: update installation instructions ## Pull Request Process +### Pull Request Guidelines + +**Keep pull requests focused and minimal.** + +PRs that touch a large number of files (50+) without clear functional justification will likely be rejected without detailed review. + +#### Why we enforce this + +- **Reviewability**: Large PRs are effectively un-reviewable. Studies show reviewer effectiveness drops significantly after ~200-400 lines of code. A 93-file "cleanup" PR cannot receive meaningful review. +- **Git history**: Sweeping changes pollute `git blame`, making it harder to trace when and why functional changes were made. +- **Merge conflicts**: Large PRs increase the likelihood of conflicts with other contributors' work. +- **Risk**: More changed lines means more opportunities for subtle bugs, even in "safe" refactors. + +#### What to do instead + +If you have a large change in mind, break it into logical, independently-mergeable slices. For example: + +- ✅ "Replace `interface{}` with `any` across codebase" (single mechanical change, easy to verify) +- ✅ "Migrate to `strings.CutPrefix` in `internal/cli`" (scoped to one package) +- ❌ "Modernize codebase with multiple idiom updates" (too broad, impossible to review) + +For sweeping refactors or style changes, **open an issue first** to discuss the approach with maintainers before investing time in the work. + ### Changelog Generation (REQUIRED) After opening your PR, generate a changelog entry: diff --git a/internal/log/log.go b/internal/log/log.go index 5ce090c7..85b9db78 100644 --- a/internal/log/log.go +++ b/internal/log/log.go @@ -51,7 +51,7 @@ func LevelFromInt(i int) Level { } // Debug writes a debug message if the global level permits. -func Debug(l Level, format string, a ...interface{}) { +func Debug(l Level, format string, a ...any) { mu.RLock() current := level w := output @@ -63,7 +63,7 @@ func Debug(l Level, format string, a ...interface{}) { // Log writes a message unconditionally to stderr. // This is for important messages that should always be shown regardless of debug level. -func Log(format string, a ...interface{}) { +func Log(format string, a ...any) { mu.RLock() w := output mu.RUnlock() diff --git a/internal/plugins/ai/anthropic/oauth_test.go b/internal/plugins/ai/anthropic/oauth_test.go index 49bd6461..251b7125 100644 --- a/internal/plugins/ai/anthropic/oauth_test.go +++ b/internal/plugins/ai/anthropic/oauth_test.go @@ -52,7 +52,7 @@ func createExpiredToken(accessToken, refreshToken string) *util.OAuthToken { } // mockTokenServer creates a mock OAuth token server for testing -func mockTokenServer(_ *testing.T, responses map[string]interface{}) *httptest.Server { +func mockTokenServer(_ *testing.T, responses map[string]any) *httptest.Server { return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path != "/v1/oauth/token" { http.NotFound(w, r) @@ -80,7 +80,7 @@ func mockTokenServer(_ *testing.T, responses map[string]interface{}) *httptest.S w.Header().Set("Content-Type", "application/json") - if errorResp, ok := response.(map[string]interface{}); ok && errorResp["error"] != nil { + if errorResp, ok := response.(map[string]any); ok && errorResp["error"] != nil { w.WriteHeader(http.StatusBadRequest) } @@ -114,8 +114,8 @@ func TestGeneratePKCE(t *testing.T) { func TestExchangeToken_Success(t *testing.T) { // Create mock server - server := mockTokenServer(t, map[string]interface{}{ - "authorization_code": map[string]interface{}{ + server := mockTokenServer(t, map[string]any{ + "authorization_code": map[string]any{ "access_token": "test_access_token", "refresh_token": "test_refresh_token", "expires_in": 3600, @@ -161,8 +161,8 @@ func TestRefreshToken_Success(t *testing.T) { os.WriteFile(tokenPath, data, 0600) // Create mock server for refresh - server := mockTokenServer(t, map[string]interface{}{ - "refresh_token": map[string]interface{}{ + server := mockTokenServer(t, map[string]any{ + "refresh_token": map[string]any{ "access_token": "new_access_token", "refresh_token": "new_refresh_token", "expires_in": 3600, diff --git a/internal/plugins/ai/lmstudio/lmstudio.go b/internal/plugins/ai/lmstudio/lmstudio.go index 970706b3..804a4b21 100644 --- a/internal/plugins/ai/lmstudio/lmstudio.go +++ b/internal/plugins/ai/lmstudio/lmstudio.go @@ -90,7 +90,7 @@ func (c *Client) ListModels() ([]string, error) { func (c *Client) SendStream(msgs []*chat.ChatCompletionMessage, opts *domain.ChatOptions, channel chan string) (err error) { url := fmt.Sprintf("%s/chat/completions", c.ApiUrl.Value) - payload := map[string]interface{}{ + payload := map[string]any{ "messages": msgs, "model": opts.Model, "stream": true, // Enable streaming @@ -148,19 +148,19 @@ func (c *Client) SendStream(msgs []*chat.ChatCompletionMessage, opts *domain.Cha break } - var result map[string]interface{} + var result map[string]any if err = json.Unmarshal(line, &result); err != nil { continue } - var choices []interface{} + var choices []any var ok bool - if choices, ok = result["choices"].([]interface{}); !ok || len(choices) == 0 { + if choices, ok = result["choices"].([]any); !ok || len(choices) == 0 { continue } - var delta map[string]interface{} - if delta, ok = choices[0].(map[string]interface{})["delta"].(map[string]interface{}); !ok { + var delta map[string]any + if delta, ok = choices[0].(map[string]any)["delta"].(map[string]any); !ok { continue } @@ -176,7 +176,7 @@ func (c *Client) SendStream(msgs []*chat.ChatCompletionMessage, opts *domain.Cha func (c *Client) Send(ctx context.Context, msgs []*chat.ChatCompletionMessage, opts *domain.ChatOptions) (content string, err error) { url := fmt.Sprintf("%s/chat/completions", c.ApiUrl.Value) - payload := map[string]interface{}{ + payload := map[string]any{ "messages": msgs, "model": opts.Model, // Add other options from opts if supported by LM Studio @@ -208,21 +208,21 @@ func (c *Client) Send(ctx context.Context, msgs []*chat.ChatCompletionMessage, o return } - var result map[string]interface{} + var result map[string]any if err = json.NewDecoder(resp.Body).Decode(&result); err != nil { err = fmt.Errorf("failed to decode response: %w", err) return } - var choices []interface{} + var choices []any var ok bool - if choices, ok = result["choices"].([]interface{}); !ok || len(choices) == 0 { + if choices, ok = result["choices"].([]any); !ok || len(choices) == 0 { err = fmt.Errorf("invalid response format: missing or empty choices") return } - var message map[string]interface{} - if message, ok = choices[0].(map[string]interface{})["message"].(map[string]interface{}); !ok { + var message map[string]any + if message, ok = choices[0].(map[string]any)["message"].(map[string]any); !ok { err = fmt.Errorf("invalid response format: missing message in first choice") return } @@ -238,7 +238,7 @@ func (c *Client) Send(ctx context.Context, msgs []*chat.ChatCompletionMessage, o func (c *Client) Complete(ctx context.Context, prompt string, opts *domain.ChatOptions) (text string, err error) { url := fmt.Sprintf("%s/completions", c.ApiUrl.Value) - payload := map[string]interface{}{ + payload := map[string]any{ "prompt": prompt, "model": opts.Model, // Add other options from opts if supported by LM Studio @@ -270,20 +270,20 @@ func (c *Client) Complete(ctx context.Context, prompt string, opts *domain.ChatO return } - var result map[string]interface{} + var result map[string]any if err = json.NewDecoder(resp.Body).Decode(&result); err != nil { err = fmt.Errorf("failed to decode response: %w", err) return } - var choices []interface{} + var choices []any var ok bool - if choices, ok = result["choices"].([]interface{}); !ok || len(choices) == 0 { + if choices, ok = result["choices"].([]any); !ok || len(choices) == 0 { err = fmt.Errorf("invalid response format: missing or empty choices") return } - if text, ok = choices[0].(map[string]interface{})["text"].(string); !ok { + if text, ok = choices[0].(map[string]any)["text"].(string); !ok { err = fmt.Errorf("invalid response format: missing or non-string text in first choice") return } @@ -294,7 +294,7 @@ func (c *Client) Complete(ctx context.Context, prompt string, opts *domain.ChatO func (c *Client) GetEmbeddings(ctx context.Context, input string, opts *domain.ChatOptions) (embeddings []float64, err error) { url := fmt.Sprintf("%s/embeddings", c.ApiUrl.Value) - payload := map[string]interface{}{ + payload := map[string]any{ "input": input, "model": opts.Model, // Add other options from opts if supported by LM Studio diff --git a/internal/plugins/ai/ollama/ollama.go b/internal/plugins/ai/ollama/ollama.go index 2cfff31d..5ab36278 100644 --- a/internal/plugins/ai/ollama/ollama.go +++ b/internal/plugins/ai/ollama/ollama.go @@ -155,7 +155,7 @@ func (o *Client) createChatRequest(ctx context.Context, msgs []*chat.ChatComplet } } - options := map[string]interface{}{ + options := map[string]any{ "temperature": opts.Temperature, "presence_penalty": opts.PresencePenalty, "frequency_penalty": opts.FrequencyPenalty, diff --git a/internal/plugins/ai/openai/openai_image_test.go b/internal/plugins/ai/openai/openai_image_test.go index 7b3135ca..c374db2b 100644 --- a/internal/plugins/ai/openai/openai_image_test.go +++ b/internal/plugins/ai/openai/openai_image_test.go @@ -345,7 +345,7 @@ func TestAddImageGenerationToolWithUserParameters(t *testing.T) { tests := []struct { name string opts *domain.ChatOptions - expected map[string]interface{} + expected map[string]any }{ { name: "All parameters specified", @@ -356,7 +356,7 @@ func TestAddImageGenerationToolWithUserParameters(t *testing.T) { ImageBackground: "transparent", ImageCompression: 0, // Not applicable for PNG }, - expected: map[string]interface{}{ + expected: map[string]any{ "size": "1536x1024", "quality": "high", "background": "transparent", @@ -372,7 +372,7 @@ func TestAddImageGenerationToolWithUserParameters(t *testing.T) { ImageBackground: "opaque", ImageCompression: 75, }, - expected: map[string]interface{}{ + expected: map[string]any{ "size": "1024x1024", "quality": "medium", "background": "opaque", @@ -386,7 +386,7 @@ func TestAddImageGenerationToolWithUserParameters(t *testing.T) { ImageFile: "/tmp/test.webp", ImageQuality: "low", }, - expected: map[string]interface{}{ + expected: map[string]any{ "quality": "low", "output_format": "webp", }, @@ -396,7 +396,7 @@ func TestAddImageGenerationToolWithUserParameters(t *testing.T) { opts: &domain.ChatOptions{ ImageFile: "/tmp/test.png", }, - expected: map[string]interface{}{ + expected: map[string]any{ "output_format": "png", }, }, diff --git a/internal/plugins/db/fsdb/storage.go b/internal/plugins/db/fsdb/storage.go index 2dc76bf8..af3d3b82 100644 --- a/internal/plugins/db/fsdb/storage.go +++ b/internal/plugins/db/fsdb/storage.go @@ -134,7 +134,7 @@ func (o *StorageEntity) buildFileName(name string) string { return fmt.Sprintf("%s%v", name, o.FileExtension) } -func (o *StorageEntity) SaveAsJson(name string, item interface{}) (err error) { +func (o *StorageEntity) SaveAsJson(name string, item any) (err error) { var jsonString []byte if jsonString, err = json.Marshal(item); err == nil { err = o.Save(name, jsonString) @@ -145,7 +145,7 @@ func (o *StorageEntity) SaveAsJson(name string, item interface{}) (err error) { return err } -func (o *StorageEntity) LoadAsJson(name string, item interface{}) (err error) { +func (o *StorageEntity) LoadAsJson(name string, item any) (err error) { var content []byte if content, err = o.Load(name); err != nil { return diff --git a/internal/plugins/template/extension_executor_test.go b/internal/plugins/template/extension_executor_test.go index bfc235a8..4f9c2dfe 100644 --- a/internal/plugins/template/extension_executor_test.go +++ b/internal/plugins/template/extension_executor_test.go @@ -187,7 +187,7 @@ esac` executor := NewExtensionExecutor(registry) // Helper function to create and register extension - createExtension := func(name, opName, cmdTemplate string, config map[string]interface{}) error { + createExtension := func(name, opName, cmdTemplate string, config map[string]any) error { configPath := filepath.Join(tmpDir, name+".yaml") configContent := `name: ` + name + ` executable: ` + testScript + ` @@ -216,7 +216,7 @@ config: // Test basic fixed file output t.Run("BasicFixedFile", func(t *testing.T) { outputFile := filepath.Join(tmpDir, "output.txt") - config := map[string]interface{}{ + config := map[string]any{ "output_file": `"output.txt"`, "work_dir": `"` + tmpDir + `"`, "cleanup": "true", @@ -241,7 +241,7 @@ config: // Test no work_dir specified t.Run("NoWorkDir", func(t *testing.T) { - config := map[string]interface{}{ + config := map[string]any{ "output_file": `"direct-output.txt"`, "cleanup": "true", } @@ -263,7 +263,7 @@ config: outputFile := filepath.Join(tmpDir, "cleanup-test.txt") // Test with cleanup enabled - config := map[string]interface{}{ + config := map[string]any{ "output_file": `"cleanup-test.txt"`, "work_dir": `"` + tmpDir + `"`, "cleanup": "true", @@ -307,7 +307,7 @@ config: // Test error cases t.Run("ErrorCases", func(t *testing.T) { outputFile := filepath.Join(tmpDir, "error-test.txt") - config := map[string]interface{}{ + config := map[string]any{ "output_file": `"error-test.txt"`, "work_dir": `"` + tmpDir + `"`, "cleanup": "true", @@ -341,7 +341,7 @@ config: // Test with missing output_file t.Run("MissingOutputFile", func(t *testing.T) { - config := map[string]interface{}{ + config := map[string]any{ "work_dir": `"` + tmpDir + `"`, "cleanup": "true", } diff --git a/internal/plugins/template/extension_registry.go b/internal/plugins/template/extension_registry.go index 67a1d155..38906427 100644 --- a/internal/plugins/template/extension_registry.go +++ b/internal/plugins/template/extension_registry.go @@ -30,7 +30,7 @@ type ExtensionDefinition struct { Operations map[string]OperationConfig `yaml:"operations"` // Additional config - Config map[string]interface{} `yaml:"config"` + Config map[string]any `yaml:"config"` } type OperationConfig struct { @@ -53,7 +53,7 @@ type ExtensionRegistry struct { // Helper methods for Config access func (e *ExtensionDefinition) GetOutputMethod() string { - if output, ok := e.Config["output"].(map[string]interface{}); ok { + if output, ok := e.Config["output"].(map[string]any); ok { if method, ok := output["method"].(string); ok { return method } @@ -61,9 +61,9 @@ func (e *ExtensionDefinition) GetOutputMethod() string { return "stdout" // default to stdout if not specified } -func (e *ExtensionDefinition) GetFileConfig() map[string]interface{} { - if output, ok := e.Config["output"].(map[string]interface{}); ok { - if fileConfig, ok := output["file_config"].(map[string]interface{}); ok { +func (e *ExtensionDefinition) GetFileConfig() map[string]any { + if output, ok := e.Config["output"].(map[string]any); ok { + if fileConfig, ok := output["file_config"].(map[string]any); ok { return fileConfig } } diff --git a/internal/plugins/template/template.go b/internal/plugins/template/template.go index 9a660cec..cfdd3df5 100644 --- a/internal/plugins/template/template.go +++ b/internal/plugins/template/template.go @@ -33,7 +33,7 @@ func init() { var pluginPattern = regexp.MustCompile(`\{\{plugin:([^:]+):([^:]+)(?::([^}]+))?\}\}`) var extensionPattern = regexp.MustCompile(`\{\{ext:([^:]+):([^:]+)(?::([^}]+))?\}\}`) -func debugf(format string, a ...interface{}) { +func debugf(format string, a ...any) { debuglog.Debug(debuglog.Trace, format, a...) } diff --git a/internal/server/models.go b/internal/server/models.go index 97b877e1..7ceddc60 100755 --- a/internal/server/models.go +++ b/internal/server/models.go @@ -24,7 +24,7 @@ func (h *ModelsHandler) GetModelNames(c *gin.Context) { return } - response := make(map[string]interface{}) + response := make(map[string]any) vendors := make(map[string][]string) for _, groupItems := range vendorsModels.GroupsItems {