do not use global flags to configure klog#312
Conversation
WalkthroughReplaces global flag parsing in pkg/common/init.go with a dedicated local FlagSet wired to klog. Sets verbosity and optional file logging via the FlagSet, and parses an empty argument list to ignore CLI args. Removes reliance on global flag.Parse while keeping Init signature unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Init as Init(init.InitLib)
participant FS as flag.FlagSet (local)
participant K as klog
App->>Init: Init(initLib)
Init->>FS: NewFlagSet("deepgram-go-sdk", ContinueOnError)
Init->>K: InitFlags(FS)
Init->>FS: Set("v", initLib.LogLevel)
alt DebugFilePath provided
Init->>FS: Set("logtostderr", "false")
Init->>FS: Set("log_file", initLib.DebugFilePath)
else No file path
Init->>FS: Set("logtostderr", "true")
end
note over Init,FS: Parse empty args to avoid global CLI args
Init->>FS: Parse([]string{})
Init-->>App: return
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNone found. Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/common/init.go (3)
37-40: Nit: drop the redundant int64 cast in FormatInt.
LogLevelis already anint64(see pkg/common/constants.go), so the extra cast is unnecessary.- err := fs.Set("v", strconv.FormatInt(int64(init.LogLevel), 10)) + err := fs.Set("v", strconv.FormatInt(init.LogLevel, 10))
42-51: Optional: pre-create the log directory and send error messages to stderr (with newline).
- If
DebugFilePathpoints to a directory that does not exist, initial writes will fail silently until first log. Pre-creating the directory provides earlier feedback.- Printing to stdout for errors in a library is surprising; prefer stderr and include a newline to avoid run-together messages.
if init.DebugFilePath != "" { - err = fs.Set("logtostderr", "false") + err = fs.Set("logtostderr", "false") if err != nil { - fmt.Printf("Error setting logtostderr: %v", err) + fmt.Fprintf(os.Stderr, "deepgram-go-sdk: error setting logtostderr: %v\n", err) } + // Ensure the directory exists before klog attempts to open the file. + if mkErr := os.MkdirAll(filepath.Dir(init.DebugFilePath), 0o755); mkErr != nil { + fmt.Fprintf(os.Stderr, "deepgram-go-sdk: failed to create log directory for %q: %v\n", init.DebugFilePath, mkErr) + } - err = fs.Set("log_file", init.DebugFilePath) + err = fs.Set("log_file", init.DebugFilePath) if err != nil { - fmt.Printf("Error setting log_file: %v", err) + fmt.Fprintf(os.Stderr, "deepgram-go-sdk: error setting log_file: %v\n", err) } }Additional changes required outside this hunk:
// add to imports import ( "os" "path/filepath" )
53-53: Nit: add a clarifying comment about parsing an empty arg list.A short comment helps future readers understand why CLI args are intentionally ignored here.
+ // Intentionally parse an empty argument list so we don't consume the program's flags. _ = fs.Parse([]string{})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
pkg/common/init.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/common/init.go (1)
pkg/common/constants.go (1)
LogLevel(16-16)
🔇 Additional comments (2)
pkg/common/init.go (2)
33-36: Good fix: klog isolated to a private FlagSet (no global flag pollution).Creating a dedicated FlagSet and wiring klog to it is the right approach to avoid clobbering program/
go testflags. This directly addresses the linked issue without altering the public API.
33-53: No global flag parsing remains
Searches forflag.Parse,pflag.Parse, and any use offlag.CommandLineoutside ofpkg/common/init.goreturned no matches. The only call toklog.InitFlagsis inpkg/common/init.go(line 35), confirming that all flag setup is confined to the library’s ownFlagSet.
1 participant
Fixes #295.
Proposed changes
Given the example go prog:
Calling
common.Initoverrides the prog flags:This PR fixes the klog configuration to not touch global flags.
Types of changes
What types of changes does your code introduce to the community Go SDK?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
Summary by CodeRabbit
Refactor
Bug Fixes
Chores