warning: support new option to override default #216
Conversation
|
WIP because while the code is {mostly, hopefully} straightforward, there are quite some design concepts to properly evaluate here. Also, tests are completely missing and we need at least basic coverage. |
pkg/context/context.go
Outdated
| @@ -18,12 +18,15 @@ type Context struct { | |||
| SnapshotPath string | |||
| SnapshotRoot string | |||
| SnapshotExclusive bool | |||
| SnapshotPreserve bool | |||
There was a problem hiding this comment.
bad rebase, will fix in the next upload which I need to do anyway to add tests.
There was a problem hiding this comment.
@fromanirh you still working on this patch, right?
There was a problem hiding this comment.
yes! totally ongoing, but not sure I can make it for end of the week (aka 0.7 cutoff).
ffromani
force-pushed
the
warning-machinery
branch
from
3c98100 to
8083c9b
Compare
ffromani
changed the title
|
Ok, managed to do some of the updates I planned and I'm reasonnably happy with the state of the PR. We can iterate and improve a bit later; WIP removed, PR fully reviewable! |
This patch add support for a new option, which allow the caller code to pass a new Alerter to ghw. `Alerter` is a dead-simple interface, supporting only a `Printf` method much like the stdlib `log` package. ghw will send all the warnings to this method. We use a so simple interface because we want to generalize a bit the existing `util.Warn` concept, but is not clear if ghw will benefit of a more complete logging interface; should that be the case, we can build on the concept we laid in this patch. Client code can programmatically override the Alerter, or users can using the faimiliar `GHW_DISABLE_WARNINGS` environ variable, which keep functioning as usual. ghw can be configured to swallow all the warnings or to keep emitting them on stderr, like it used to do up until this patch. Signed-off-by: Francesco Romani <[email protected]>
ffromani
force-pushed
the
warning-machinery
branch
from
8083c9b to
21cfac3
Compare
|
fixed stale commit message. No code changes. |
Make it easier for users of the ghw API to disable warnings providing a `NullAlerter`. Signed-off-by: Francesco Romani <[email protected]>
|
|
||
| You may also supply an `Alerter` to ghw to handle the warnings. | ||
| In this case, please check the `option.Alerter` interface (compatible with stdlib's | ||
| log.Logger type) and the `ghw.WithAlerter()` function | ||
| ``` |
|
just wild thought reviewing #175: do we want to add an alias, perhaps in another PR, to minimize the surprise for users? Programmatically we have I believe we should keep WithNullAlerter because it's more powerful construct. |
Sure! Go for it :) |
With this patch we make the ghw API more consistent, matching the more generic Alerter subsystem with the existing `GHW_DISABLE_WARNINGS` environ variables, with minimal cost and with the added benefit to minimize users' surprise. Signed-off-by: Francesco Romani <[email protected]>
|
Done! Now I think we have everything we need to call #175 really done. |
This PR wants to solve #175
This patch add support for a new option, which allow
the caller code to pass a new Alerter to ghw.
Alerteris a dead-simple interface, supporting only aPrintfmethod much like the stdliblogpackage.ghw will send all the warnings to this method.
We use a so simple interface because we want to generalize a bit
the existing
util.Warnconcept, but is not clear if ghw will benefitof a more complete logging interface; should that be the case, we
can build on the concept we laid in this patch.
Client code can programmatically override the Alerter, or
users can using the faimiliar
GHW_DISABLE_WARNINGSenviron variable,which keep functioning as usual.
ghw can be configured to swallow all the warnings or to keep emitting them
on stderr, like it used to do up until this patch.
Signed-off-by: Francesco Romani [email protected]