Refactor: Extract scattered helper functions into dedicated files#3601
Refactor: Extract scattered helper functions into dedicated files#3601
Conversation
- Created pkg/workflow/config_helpers.go with config parsing functions - Created pkg/workflow/frontmatter_helpers.go with frontmatter utilities - Moved parseLabelsFromConfig, parseTitlePrefixFromConfig, parseTargetRepoFromConfig, parseStringFromConfig from config.go - Moved extractStringValue, parseIntValue, filterMapKeys from frontmatter_extraction.go - All tests passing (make test-unit) - Build successful (make build) - Code formatted and linted (make fmt, make lint) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the codebase to improve organization by extracting scattered helper functions from config.go and frontmatter_extraction.go into dedicated helper files. The refactoring maintains all existing functionality without changing function signatures or test coverage.
- Created two new helper files with clear, focused purposes for configuration and frontmatter parsing utilities
- Moved generic helper functions from their scattered locations to dedicated files
- All functions remain in the same
workflowpackage with unchanged signatures, ensuring no breaking changes
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/workflow/config_helpers.go | New file containing configuration parsing helpers (parseLabelsFromConfig, parseTitlePrefixFromConfig, parseTargetRepoFromConfig, parseStringFromConfig) |
| pkg/workflow/frontmatter_helpers.go | New file containing frontmatter utility functions (extractStringValue, parseIntValue, filterMapKeys) |
| pkg/workflow/config.go | Reduced to placeholder file with comment indicating functions moved to config_helpers.go |
| pkg/workflow/frontmatter_extraction.go | Removed three helper functions with note about their relocation to frontmatter_helpers.go |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| package workflow | ||
|
|
||
| import ( | ||
| "github.com/githubnext/gh-aw/pkg/logger" | ||
| ) | ||
|
|
||
| var configHelpersLog = logger.New("workflow:config_helpers") |
There was a problem hiding this comment.
[nitpick] Consider adding a package-level documentation comment to explain the purpose of this file. For example:
package workflow
import (
"github.com/githubnext/gh-aw/pkg/logger"
)
// This file contains helper functions for parsing and extracting values from
// safe output configuration maps used in workflow compilation.
var configHelpersLog = logger.New("workflow:config_helpers")This would improve discoverability and help developers understand the file's purpose at a glance.
| package workflow | ||
|
|
||
| import ( | ||
| "github.com/githubnext/gh-aw/pkg/logger" | ||
| ) | ||
|
|
||
| var configLog = logger.New("workflow:config") | ||
|
|
||
| // parseLabelsFromConfig extracts and validates labels from a config map | ||
| // Returns a slice of label strings, or nil if labels is not present or invalid | ||
| func parseLabelsFromConfig(configMap map[string]any) []string { | ||
| if labels, exists := configMap["labels"]; exists { | ||
| configLog.Print("Parsing labels from config") | ||
| if labelsArray, ok := labels.([]any); ok { | ||
| var labelStrings []string | ||
| for _, label := range labelsArray { | ||
| if labelStr, ok := label.(string); ok { | ||
| labelStrings = append(labelStrings, labelStr) | ||
| } | ||
| } | ||
| // Return the slice even if empty (to distinguish from not provided) | ||
| if labelStrings == nil { | ||
| configLog.Print("No valid label strings found, returning empty array") | ||
| return []string{} | ||
| } | ||
| configLog.Printf("Parsed %d labels from config", len(labelStrings)) | ||
| return labelStrings | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // parseStringFromConfig is a generic helper that extracts and validates a string value from a config map | ||
| // Returns the string value, or empty string if not present or invalid | ||
| func parseStringFromConfig(configMap map[string]any, key string) string { | ||
| if value, exists := configMap[key]; exists { | ||
| if valueStr, ok := value.(string); ok { | ||
| configLog.Printf("Parsed %s from config: %s", key, valueStr) | ||
| return valueStr | ||
| } | ||
| } | ||
| return "" | ||
| } | ||
|
|
||
| // parseTitlePrefixFromConfig extracts and validates title-prefix from a config map | ||
| // Returns the title prefix string, or empty string if not present or invalid | ||
| func parseTitlePrefixFromConfig(configMap map[string]any) string { | ||
| return parseStringFromConfig(configMap, "title-prefix") | ||
| } | ||
|
|
||
| // parseTargetRepoFromConfig extracts the target-repo value from a config map. | ||
| // Returns the target repository slug as a string, or empty string if not present or invalid. | ||
| // This function does not perform any special handling or validation for wildcard values ("*"); | ||
| // callers are responsible for validating the returned value as needed. | ||
| func parseTargetRepoFromConfig(configMap map[string]any) string { | ||
| return parseStringFromConfig(configMap, "target-repo") | ||
| } | ||
| // Configuration parsing helpers have been moved to config_helpers.go | ||
| // This file is kept as a placeholder for any future config-related functionality |
There was a problem hiding this comment.
This file should be removed rather than kept as an empty placeholder. According to the project's coding guidelines on file organization, empty placeholder files are considered an anti-pattern. Since all the configuration parsing helpers have been moved to config_helpers.go, this file no longer serves a purpose and keeping it adds unnecessary clutter to the codebase.
If configuration-related functionality needs to be added in the future, a new file can be created at that time with actual content.
See below for a potential fix:
| package workflow | ||
|
|
There was a problem hiding this comment.
[nitpick] Consider adding a package-level documentation comment to explain the purpose of this file, similar to how other helper files in the codebase are documented. For example:
package workflow
// This file contains generic utility functions for extracting and parsing
// values from frontmatter maps used throughout the workflow compilation process.This would improve discoverability and help developers understand the file's purpose at a glance.
See below for a potential fix:
@ -1,3 +1,5 @@
// This file contains generic utility functions for extracting and parsing
// values from frontmatter maps used throughout the workflow compilation process.
Generic helper functions were scattered across
config.goandfrontmatter_extraction.gowithout clear organizational pattern, making them difficult to discover and maintain.Changes
Created helper files:
pkg/workflow/config_helpers.go- Configuration parsing helpers (parseLabelsFromConfig,parseTitlePrefixFromConfig,parseTargetRepoFromConfig,parseStringFromConfig)pkg/workflow/frontmatter_helpers.go- Frontmatter utilities (extractStringValue,parseIntValue,filterMapKeys)Cleaned up original files:
pkg/workflow/config.go- Removed helper functions (58→4 lines)pkg/workflow/frontmatter_extraction.go- Removed generic utilities, retained compiler-specific methodsAll functions remain in the
workflowpackage with unchanged signatures. Existing test coverage maintained without modification.Example
Related to #3435
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.