-
Notifications
You must be signed in to change notification settings - Fork 8
feat(main): Use cobra for commandline handling #548
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
📝 WalkthroughWalkthroughThis pull request introduces a Cobra-based CLI framework to replace the standard library Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches✅ Passed checks (4 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 |
Signed-off-by: Akhil Repala <arepala@blinklabs.io>
29d73ed to
a1cd436
Compare
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
194-210: Inconsistent help flag references in filtering section.This section still references
-h/-helpand shows old-style flag output format, while the updated help section at line 111-128 now uses--helpand Cobra's output format. Consider updating this section for consistency.-You can get a list of all available filter options by using the `-h`/`-help` +You can get a list of all available filter options by using the `--help` flag. ```bash -$ ./adder -h -Usage of adder: +$ ./adder --help +Usage: + adder [flags] + +Flags: ... - -filter-address string + --filter-address string specifies address to filter on - -filter-asset string + --filter-asset string specifies the asset fingerprint (asset1xxx) to filter on - -filter-policy string + --filter-policy string specifies asset policy ID to filter on - -filter-type string + --filter-type string specifies event type to filter on ...</blockquote></details> </blockquote></details>🧹 Nitpick comments (2)
README.md (1)
61-99: Inconsistent JSON indentation in transaction example.The
blockandrollbackexamples were updated to use 2-space indentation, but thetransactionexample still uses 4-space indentation. Consider normalizing for consistency across all examples.cmd/adder/main.go (1)
143-159: Consider returning error instead of callingos.Exitin goroutine.This
os.Exit(1)in the goroutine (line 157) is inconsistent with the PR's goal of replacing exit-based error handling with error propagation. It bypasses graceful shutdown and Cobra's error handling.Consider using an error channel pattern or making the debug listener startup synchronous so errors can be returned:
// Start debug listener if cfg.Debug.ListenPort > 0 { logger.Info(fmt.Sprintf( "starting debug listener on %s:%d", cfg.Debug.ListenAddress, cfg.Debug.ListenPort, )) + debugger := &http.Server{ + Addr: fmt.Sprintf( + "%s:%d", + cfg.Debug.ListenAddress, + cfg.Debug.ListenPort, + ), + ReadHeaderTimeout: 60 * time.Second, + } go func() { - debugger := &http.Server{ - Addr: fmt.Sprintf( - "%s:%d", - cfg.Debug.ListenAddress, - cfg.Debug.ListenPort, - ), - ReadHeaderTimeout: 60 * time.Second, - } err := debugger.ListenAndServe() - if err != nil { - logger.Error( - fmt.Sprintf("failed to start debug listener: %s", err), - ) - os.Exit(1) + if err != nil && err != http.ErrServerClosed { + logger.Error(fmt.Sprintf("debug listener error: %s", err)) } }() }Alternatively, if immediate failure is critical, start the listener synchronously before the goroutine to catch binding errors early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum📒 Files selected for processing (6)
README.md(5 hunks)cmd/adder/main.go(8 hunks)go.mod(1 hunks)internal/config/config.go(3 hunks)plugin/option.go(2 hunks)plugin/register.go(2 hunks)🧰 Additional context used
🧬 Code graph analysis (1)
cmd/adder/main.go (4)
internal/config/config.go (1)
GetConfig(173-175)internal/version/version.go (2)
Version(23-23)GetVersionString(27-33)plugin/register.go (5)
GetPlugins(103-111)PluginTypeInput(26-26)PluginTypeOutput(27-27)ProcessConfig(86-101)ProcessEnvVars(69-84)plugin/plugin.go (1)
Plugin(21-27)🔇 Additional comments (8)
plugin/register.go (1)
17-21: LGTM - clean pflag migration.The import and function signature changes are minimal and correct. The
pflaglibrary maintains API compatibility with the standardflagpackage for these operations, so no logic changes are needed.Also applies to: 58-67
plugin/option.go (1)
22-24: LGTM - straightforward pflag migration.The migration correctly updates the import and method signature. The
pflaglibrary'sStringVar,BoolVar,IntVar, andUintVarmethods are API-compatible with the standard library, so the flag registration logic remains unchanged.Also applies to: 45-88
internal/config/config.go (1)
154-170: LGTM - clean refactor to external flag binding.The
BindFlagsmethod correctly delegates flag registration to the providedpflag.FlagSet, allowing Cobra to handle parsing. This is the appropriate pattern for Cobra integration. The--versionflag works correctly: it's registered inBindFlags, parsed by Cobra, and checked inrun()at line 87 to print the version string.cmd/adder/main.go (5)
41-52: LGTM!The Cobra command setup is well-structured:
SilenceUsage: trueprevents printing usage on every error, which is appropriate for a daemon-style application.cobra.NoArgscorrectly enforces that no positional arguments are accepted.- Delegating to
run()viaRunEenables clean error propagation.
81-84: LGTM!Using
panicfor flag binding errors ininit()is acceptable here since these are configuration errors that indicate a programming mistake rather than a runtime condition.
86-134: LGTM!The refactored error handling using
fmt.Errorfwith%wfor error wrapping is clean and idiomatic. Early returns for--versionand list commands are appropriate.
213-221: LGTM!The non-blocking error consumer pattern is appropriate here. The comment clearly documents the intentional behavior of logging pipeline errors without terminating, which allows the application to continue operating through transient errors.
237-241: LGTM!The
main()function correctly follows the standard Cobra pattern: execute the root command and exit with status 1 on error. Cobra handles error printing internally.
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 7 files
adder --helpoutput.Closes #164
Summary by cubic
Switched the CLI to a Cobra root command for consistent parsing and help output. Migrated all config/plugin flags to pflag and updated README to match the new adder --help.
Refactors
Dependencies
Written for commit a1cd436. Summary will update automatically on new commits.
Summary by CodeRabbit
Documentation
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.