Eliminate three duplicate code patterns in logger and config packages#1120
Eliminate three duplicate code patterns in logger and config packages#1120
Conversation
Pattern 1 (#1106): Replace switch-level dispatch blocks with levelFuncs map - Add levelFuncs map in file_logger.go mapping LogLevel to global log function - Replace 13-line switch in logWithMarkdownLevel (markdown_logger.go) - Replace 9-line switch in logWithLevelAndServer (server_file_logger.go) Pattern 2 (#1107): Eliminate duplicated gateway validation in validateStringPatterns - validateStringPatterns now delegates to validateGatewayConfig for port/timeout/payloadDir - Only domain-specific check remains in validateStringPatterns (~20 lines removed) Pattern 3 (#1108): Document the Log-Level Quad-Function Pattern in common.go - New section explains the intentional pattern and instructs contributors on adding new levels Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
|
@copilot instead of naming the map levelFuncs name it logFuncs |
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Done — renamed to Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
This PR refactors duplicated logic in the internal/logger and internal/config packages by centralizing log-level dispatch and reusing gateway validation logic, with added documentation describing an intentional wrapper-function pattern.
Changes:
- Introduce a
logFuncsmap as the singleLogLevel -> logging functiondispatch table and use it from markdown and per-server logging helpers. - Deduplicate gateway port/timeout validation by delegating from
validateStringPatternstovalidateGatewayConfig. - Add documentation describing the intentional “quad-function” wrapper pattern and how to extend it for new log levels.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/logger/file_logger.go | Adds logFuncs dispatch map for log-level to function mapping. |
| internal/logger/markdown_logger.go | Replaces per-level switch dispatch with logFuncs[level]. |
| internal/logger/server_file_logger.go | Replaces per-level switch dispatch with logFuncs[level] (with nil guard). |
| internal/logger/common.go | Adds documentation for the log-level wrapper pattern (but currently interrupts closeLogFile docs). |
| internal/config/validation_schema.go | Delegates gateway port/timeout/payloadDir validation to validateGatewayConfig. |
Comments suppressed due to low confidence (1)
internal/logger/common.go:229
- The doc comment immediately preceding closeLogFile no longer starts with the function name (it now starts with “It syncs…”), because the new “Log-Level Quad-Function Pattern” section was inserted between the header line and the rest of the closeLogFile description. This makes the closeLogFile documentation harder to read and can trip comment-format linters. Consider restoring a single-line
closeLogFile ...doc comment immediately above the function, and moving the quad-function documentation to a separate block so it doesn’t interrupt the function doc.
// It syncs buffered data before closing and handles errors appropriately.
// The mutex should already be held by the caller.
//
// Error handling strategy:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if stdinCfg.Gateway != nil { | ||
| // Validate port: must be integer 1-65535 or variable expression | ||
| if stdinCfg.Gateway.Port != nil { | ||
| if err := rules.PortRange(*stdinCfg.Gateway.Port, "gateway.port"); err != nil { | ||
| return err | ||
| } | ||
| // Delegate port, timeout, and payloadDir validation to validateGatewayConfig | ||
| // to avoid duplicating those checks here. | ||
| if err := validateGatewayConfig(stdinCfg.Gateway); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
validateStringPatterns now calls validateGatewayConfig. LoadFromStdin already calls validateGatewayConfig later in the parsing pipeline, so this change causes gateway validation (and associated logging) to run twice for stdin configs. Consider keeping validateStringPatterns focused on regex/pattern checks (e.g., the domain rules) and letting the single validateGatewayConfig call remain in the main validation flow, or otherwise ensuring validateGatewayConfig is only invoked once per load.
See below for a potential fix:
// Validate gateway configuration patterns (domain rules only; structural checks are handled elsewhere)
if stdinCfg.Gateway != nil {
| func logWithMarkdownLevel(level LogLevel, category, format string, args ...interface{}) { | ||
| var regularLogFunc func(string, string, ...interface{}) | ||
|
|
||
| switch level { | ||
| case LogLevelInfo: | ||
| regularLogFunc = LogInfo | ||
| case LogLevelWarn: | ||
| regularLogFunc = LogWarn | ||
| case LogLevelError: | ||
| regularLogFunc = LogError | ||
| case LogLevelDebug: | ||
| regularLogFunc = LogDebug | ||
| } | ||
|
|
||
| logWithMarkdown(level, regularLogFunc, category, format, args...) | ||
| logWithMarkdown(level, logFuncs[level], category, format, args...) | ||
| } |
There was a problem hiding this comment.
logWithMarkdownLevel passes logFuncs[level] directly into logWithMarkdown, and logWithMarkdown unconditionally calls the provided regularLogFunc. If a new LogLevel is added (or an unexpected value is passed) without a corresponding logFuncs entry, this will panic at runtime. Consider guarding the lookup (ok/nil check) and either defaulting to a safe logger (e.g., LogInfo/LogDebug) or skipping the regular logger call while still logging to the markdown logger.
Static analysis identified ~60–70 duplicated lines across
internal/loggerandinternal/config: two identicalswitch leveldispatch blocks, duplicated gateway validation logic split across two files, and a structural quad-function pattern repeated three times with no single update point.Changes
logFuncsmap replacesswitch levelblocksAdded a
logFuncsmap infile_logger.goas the singleLogLevel → log functiondispatch table. BothlogWithMarkdownLevelandlogWithLevelAndServernow look up from it instead of maintaining independent switch blocks — adding a new log level now requires one edit.validateStringPatternsdelegates tovalidateGatewayConfigPort-range and timeout checks were duplicated between
validateGatewayConfig(validation.go) andvalidateStringPatterns(validation_schema.go).validateStringPatternsnow callsvalidateGatewayConfigfor those fields, retaining only the domain check (which is unique to it). Removes ~20 duplicated lines and makesvalidateGatewayConfigthe single source of truth for gateway field validation.Quad-function pattern documented in
common.goAdded a documentation section (matching the style of the existing "Close Pattern" and "Initialization Pattern" docs) explaining the intentional three-set quad-function pattern and instructing contributors to update
logFuncsplus all three wrapper files when adding a new log level.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.