Return from handle_message as to if message was handled or not #25
Conversation
Codecov Report
@@ Coverage Diff @@
## master #25 +/- ##
==========================================
- Coverage 78.94% 76.47% -2.48%
==========================================
Files 7 8 +1
Lines 57 68 +11
==========================================
+ Hits 45 52 +7
- Misses 12 16 +4
Continue to review full report at Codecov.
|
| val :: Bool | ||
| end | ||
|
|
||
| was_handled(response) = true # If we don;t get a MessageHandled then assume worked |
There was a problem hiding this comment.
I wonder if it makes sense to return missing here so that tee logger would use three-valued logic automatically. The caller can then distinguish "definitely handled", "maybe handled", "not handled." Example use-case may be that, in FirstMatchLogger #23:
!== falseis consideredtruefor info level and below- but only
== trueis consideredtruefor more urgent messages (to make sure they are shown)
Not sure it's worth the complexity, though.
There was a problem hiding this comment.
Oooh interesting idea. Worth consideration
There was a problem hiding this comment.
Or could it make sense to just return an Enum value? Then we have the option to expand the possible statuses later if necessary. On the downside making that forward compatible could be annoying.
If people always write utility routing tools in terms of was_handled (or other verbs we provide) forward compatibility would be easier. Can we somehow arrange was_handled to be the main API with the type MessageHandled as secondary?
There was a problem hiding this comment.
Conversely: maybe was_handled should just be moved into TeeLogger
Right now it is only used to Booleanize the status so that can be written using handled |= was_handled(status)
|
There was a problem hiding this comment.
I didn't see this until just now sorry!
This is thought provoking thanks. The biggest question for me is what verbs should act on the "return status type". Personally I'm not sure I'd like to subscribe to the particular three-valued logic of missing, though if we did decide it had the right semantic it would be a nice win to have less types.
src/common.jl
Outdated
| if it did not actually handle the log message. | ||
| For example, if the log message was below the level it should log. | ||
| This is of particular relevance to the [`ActiveFilteredLogger`](@ref), which can't know | ||
| til `handle_message` if a log message will be filtered or not. |
There was a problem hiding this comment.
| til `handle_message` if a log message will be filtered or not. | |
| util `handle_message` if a log message will be filtered or not. |
There was a problem hiding this comment.
| til `handle_message` if a log message will be filtered or not. | |
| until `handle_message` if a log message will be filtered or not. |
| This is of particular relevance to the [`ActiveFilteredLogger`](@ref), which can't know | ||
| til `handle_message` if a log message will be filtered or not. | ||
| Ideally, `MessageHandled(true)` would be returned from loggers when when they | ||
| successfully handled a message, however this is not strictly required. |
There was a problem hiding this comment.
But if a sink unsuccessfully handles a message, what then? Maybe most log routing networks should routinely be set up to have a safe-as-possible fallback of "just print synchronously to the console if all else fails"? Then there would be meaning for a sink returning MessageHandled(false)?
There was a problem hiding this comment.
(Tangentially related thought: a composable safe fallback logger could allow us to move the try/catch logic which is currently hardcoded in the frontend out into a backend.)
There was a problem hiding this comment.
But if a sink unsuccessfully handles a message, what then?
I imaging mostly that would result in an exception (e.g. could not write to file)
but that might not be a good thing.
(Tangentially related thought: a composable safe fallback logger could allow us to move the try/catch logic which is currently hardcoded in the frontend out into a backend.)
Something like #12
| val :: Bool | ||
| end | ||
|
|
||
| was_handled(response) = true # If we don;t get a MessageHandled then assume worked |
There was a problem hiding this comment.
Or could it make sense to just return an Enum value? Then we have the option to expand the possible statuses later if necessary. On the downside making that forward compatible could be annoying.
If people always write utility routing tools in terms of was_handled (or other verbs we provide) forward compatibility would be easier. Can we somehow arrange was_handled to be the main API with the type MessageHandled as secondary?
Co-Authored-By: Chris Foster <[email protected]>
Will close #23
I am not sure on the name of the struct.
Possibly should be
HandlerResponse?cc @tkf