Conversation
grayside
left a comment
grayside
left a comment
There was a problem hiding this comment.
Overall good stuff. Should the logo be moved to an images directory in case we have other uses for it?
| func (cmd *BaseCommand) Success(message string) error { | ||
| if message != "" { | ||
| cmd.out.Info.Println(message) | ||
| util.NotifySuccess(cmd.context, message) |
There was a problem hiding this comment.
Since a number of our commands do not return cmd.Success(), is the idea that not returning Success() means the command is opting out of notifications? It seems as though we should return success for everything and have a boolean parameter to suppress notifications.
There was a problem hiding this comment.
They should all be returning cmd.Success now, that was part of the PR to transition to this centralized system. If you dont want a message or notifications you just return "" the empty string.
There was a problem hiding this comment.
Okay. We may want to switch this per my comment later on if we have other things triggered on Success that we don't want commands to opt-out from. That seems fine to happen when such a use case arises.
util/notify.go
Outdated
| if !isSet { | ||
| return true | ||
| } | ||
| val, err := strconv.ParseBool(env) |
There was a problem hiding this comment.
Oooh, ParseBool. Should this be in our user input util function?
There was a problem hiding this comment.
doesn't seem like it would need to move there, there is app and use case specific knowledge in this case and just wrapping a core lib function seems like overkill.
util/notify.go
Outdated
|
|
||
| // shouldNotify returns a boolean if notifications are enabled | ||
| func shouldNotify() bool { | ||
| env, isSet := os.LookupEnv("RIG_SILENCE_NOTIFY") |
There was a problem hiding this comment.
My sense is that RIG_NOTIFY_SILENCE is a better name, as there is more likely to be other RIG_NOTIFY_* vars than RIG_SILENCE_*
There was a problem hiding this comment.
Should this be a global parameter, allowing rig --silence
There was a problem hiding this comment.
Most apps use --quiet but that is generally targeted at log messages, not notifications. I can go either way on a CLI flag for it. Do you feel strongly one way or another?
(I do agree with the name change)
| cmd.machine = Machine{Name: c.GlobalString("name"), out: util.Logger()} | ||
|
|
||
| util.NotifyInit(fmt.Sprintf("Outrigger (rig) %s", c.App.Version)) | ||
| cmd.context = c |
There was a problem hiding this comment.
Why do we need to stash context? This is begging for a why comment.
There was a problem hiding this comment.
Ah, we need it so Error() and Success() do not need ctx passed in. Could still use a comment to point that out.
|
w/r/t logo, I think that if the application itself uses it, it should be in an application/code folder, that way any changes are done in a known application space. If we need a logo for more documentation or other purposes, then we can have an additional one in an |
2 participants
Disabling it globally is controlled with
RIG_SILENCE_NOTIFYenv var that is a boolean. It takes1, t, T, TRUE, true, Truefor true and0, f, F, FALSE, false, Falsefor false