refactor: use services for commands - #1434
Conversation
|
@greptileai review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -0,0 +1,31 @@ | |||
| package app | |||
There was a problem hiding this comment.
run.go and uninstall.go both live in internal/commands/ but declare package app. In Go every file in a directory must share the same package name; the other four files in this directory (add.go, check_install.go, dump.go, install.go) all declare package commands. The build will fail until these two files are corrected to package commands.
| @@ -0,0 +1,16 @@ | |||
| package app | |||
There was a problem hiding this comment.
Same issue as run.go: this file declares package app but belongs in internal/commands/ alongside files that declare package commands. The mismatch causes a build failure. Change the declaration to package commands.
| return errMsg.String() | ||
| } |
There was a problem hiding this comment.
formatHooksPathError returns a string, not an error — compile error
errMsg.String() has type string, but the declared return type is error. strings.Builder does not implement the error interface (Error() string), so the compiler rejects this. Wrap it in errors.New or fmt.Errorf.
| return errMsg.String() | |
| } | |
| return errors.New(errMsg.String()) | |
| } |
| func (s *ConfigService) loadConfig() (*config.Config, error) { | ||
| cfg, err := config.Load(s.repo) | ||
|
|
||
| // Reset loaded colors | ||
| s.setColors(cfg.Colors) | ||
|
|
||
| return cfg, err | ||
| } |
There was a problem hiding this comment.
config.Load returns an error
config.Load(s.repo) can return (nil, err). The very next line unconditionally reads cfg.Colors, which panics on a nil cfg. The color reset should only run when cfg is non-nil.
| func (s *ConfigService) loadConfig() (*config.Config, error) { | |
| cfg, err := config.Load(s.repo) | |
| // Reset loaded colors | |
| s.setColors(cfg.Colors) | |
| return cfg, err | |
| } | |
| func (s *ConfigService) loadConfig() (*config.Config, error) { | |
| cfg, err := config.Load(s.repo) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // Reset loaded colors | |
| s.setColors(cfg.Colors) | |
| return cfg, err | |
| } |
| func (s *ConfigService) ForEachRemote(f func(remote *config.Remote)) { | ||
| cfg, err := s.Load() | ||
| if err != nil { | ||
| panic(err.Error()) | ||
| } |
There was a problem hiding this comment.
ForEachRemote panics on load error — violates project rule
AGENTS.md explicitly forbids panic in production paths. If config loading fails here the process crashes with a stack trace instead of a clean error. The function signature should return error so callers can handle it, or at minimum the panic should be replaced with a logged warning.
| func (s *ConfigService) ForEachRemote(f func(remote *config.Remote)) { | |
| cfg, err := s.Load() | |
| if err != nil { | |
| panic(err.Error()) | |
| } | |
| func (s *ConfigService) ForEachRemote(f func(remote *config.Remote)) error { | |
| cfg, err := s.Load() | |
| if err != nil { | |
| return err | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| } | ||
|
|
||
| func Run(ctx context.Context, app *app.App, args RunArgs) error { |
There was a problem hiding this comment.
Run is an unimplemented stub
The function body is return nil, meaning any invocation of lefthook run will silently succeed without executing any hooks. If this is intentional work in progress the PR description should say so; otherwise the missing implementation is a regression.
| func Uninstall(ctx context.Context, app *app.App, args UninstallArgs) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Uninstall is an unimplemented stub
The function always returns nil, so lefthook uninstall will silently do nothing. If left merged as-is, installed hooks will never be removed when users run the uninstall command.
| Action: func(ctx context.Context, cmd *cli.Command) error { | ||
| l, err := command.NewLefthook(verbose, "auto") | ||
| // l, err := command.NewLefthook(verbose, "auto") | ||
| // if err != nil { | ||
| // return err | ||
| // } | ||
| app, err := newApp(verbose, "") |
There was a problem hiding this comment.
The old command.NewLefthook call was left behind as a multi-line comment. It serves no documentation purpose and adds noise; remove it before merging.
| Action: func(ctx context.Context, cmd *cli.Command) error { | |
| l, err := command.NewLefthook(verbose, "auto") | |
| // l, err := command.NewLefthook(verbose, "auto") | |
| // if err != nil { | |
| // return err | |
| // } | |
| app, err := newApp(verbose, "") | |
| Action: func(ctx context.Context, cmd *cli.Command) error { | |
| app, err := newApp(verbose, "") |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Closes # (issue)
Context
Changes
Confidence Score: 1/5
Not safe to merge — the build does not compile and two core commands are no-ops.
Three distinct compile errors will prevent any binary from being produced. Even if those were patched,
lefthook runandlefthook uninstallboth return nil without doing anything, silently breaking the two most common user workflows. Additionally,loadConfigdereferences a potentially nil pointer andForEachRemotecan crash the process via panic on bad config.internal/commands/run.go,internal/commands/uninstall.go, andinternal/app/hooks_service.gohave compile errors.internal/app/config_service.goneeds attention for the nil-deref, panic, and unsafe type assertions.Comments Outside Diff (1)
internal/app/config_service.go, line 625-633 (link)setColorscan panicIn the
map[string]anybranch, each color is extracted withcolorsTyped["cyan"].(string)and similar. If a key is absent from the map the assertion is onnil, and if the value is not actually a string both cases produce a runtime panic. The config is user-supplied, so a missing or wrong-typed color name is entirely plausible. Use the two-value assertion form (v, ok := colorsTyped["cyan"].(string)) and skip or fall back gracefully when!ok.Reviews (1): Last reviewed commit: "chore: rebase the branch [skip ci]" | Re-trigger Greptile