feat: WithNotifySignal #48
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds a new WithNotifySignal option to let users specify OS signals that will cancel the command’s context.
- Introduces
WithNotifySignaltosettingsand sets upsignal.NotifyContextwhen signals are provided. - Updates
Executeinfang.goto wrap the root context with signal handling. - Extends the example in
example/main.goto demonstrate cancellation on interrupt and kill.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| fang.go | Add signals field, WithNotifySignal option, and wrap context with signal.NotifyContext. |
| example/main.go | Add a RunE demo command and call fang.Execute with WithNotifySignal(os.Interrupt, os.Kill). |
Comments suppressed due to low confidence (3)
example/main.go:93
- Inside the
RunEcallback, use the local command instance (c.Println) rather than the outercmdvariable to ensure output goes to the correct command’s I/O.
cmd.Println("Working...")
example/main.go:117
- Note that
os.Kill(SIGKILL) cannot be caught or intercepted. Consider using a signal likesyscall.SIGTERMfor graceful shutdown.
fang.WithNotifySignal(os.Interrupt, os.Kill),
fang.go:95
- Add unit tests for
WithNotifySignalto verify that providing signals correctly wraps the context withsignal.NotifyContextand cancels on the expected signals.
func WithNotifySignal(signals ...os.Signal) Option {
| if len(opts.signals) > 0 { | ||
| var cancel context.CancelFunc | ||
| ctx, cancel = signal.NotifyContext(ctx, opts.signals...) | ||
| defer cancel() | ||
| } |
There was a problem hiding this comment.
@caarlos0 I think this is in part not needed and in part not a responsability Fang should take.
os/signal.Notify and os/signal.NotifyContext are functions that not only notify you about signals in channels, but they also add a side-effect that the app won't be finished automatically anymore while receiving the signal. When the app calls os/signal.Notify, it has the responsibility to finish the finish itself.
This means that os/signal.Notify should basically be called always by the main app, and not by libraries.
This should also not be needed by CLI apps like Glow because the default Go behavior is what we want anyway, which is to simply finish the app. os/signal.Notify is for when you want to run something before finishing the app. (Task uses it to give processes a few seconds to finish before we kill them, for example).
not sure about this yet.
based on this discussion: charmbracelet/glow#786 (comment)