-
Notifications
You must be signed in to change notification settings - Fork 6
fix(cmd): preserve non-profiling args correctly. #1097
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
📝 WalkthroughWalkthroughThe change modifies argument processing in Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 1 file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/dingo/main.go (1)
169-175: Consider reusing the globalprogramNameconstant or renaming the local variableThe reconstruction of
os.Argswith the original program name is sound and nil‑safe. However, the localprogramNamevariable both duplicates the literal"dingo"and shadows the package‑levelconst programName. To reduce duplication and potential confusion, consider either:
- Initializing from the global constant and only overriding from
os.Args[0]when present, or- Renaming the local (e.g.,
exeName) to avoid shadowing.This keeps intent clearer without changing behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/dingo/main.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Analyze (go)
- GitHub Check: nilaway
- GitHub Check: go-test (1.25.x, ubuntu-latest)
- GitHub Check: go-test (1.24.x, macos-latest)
- GitHub Check: go-test (1.25.x, windows-latest)
- GitHub Check: go-test (1.25.x, macos-latest)
- GitHub Check: go-test (1.24.x, ubuntu-latest)
- GitHub Check: go-test (1.24.x, windows-latest)
- GitHub Check: lint
- GitHub Check: docker (ubuntu-latest, amd64)
🔇 Additional comments (1)
cmd/dingo/main.go (1)
144-167: Profiling flag filtering and arg preservation look correctThe loop now cleanly strips only the
--cpuprofile/--memprofileflags (both--flag=valueand--flag valueforms) while preserving all other arguments and their order innewArgs. This aligns with the PR objective to keep non‑profiling args intact and avoids leaking profiling flags to Cobra.
Summary by cubic
Fixed CLI parsing so non-profiling args are preserved when handling --cpuprofile and --memprofile before Cobra init. Rebuilds os.Args with the program name and filtered args to prevent commands and options from being dropped.
Written for commit 24d2bbf. Summary will update automatically on new commits.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.