Add ?fail_on_error argument to Lwt_log_core.load_rules, also expose new level_of_string function in the interface #306
Conversation
When set to true, it causes load_rules raise Failure if it fails to parse the rules, otherwise the behaviour remains the same for backwards compatibility.
| @@ -47,6 +47,17 @@ let string_of_level = function | |||
| | Error -> "error" | |||
| | Fatal -> "fatal" | |||
|
|
|||
| let level_of_string str = | |||
| let str = (String.lowercase [@ocaml.warning "-3"]) str in | |||
There was a problem hiding this comment.
You're right to worry about this, but we can't use lowercase_ascii because Lwt still supports 4.02.
There was a problem hiding this comment.
Ok, good I kept it intact then.
| | "warning" -> Ok Warning | ||
| | "error" -> Ok Error | ||
| | "fatal" -> Ok Fatal | ||
| | _ -> Pervasives.Error (Printf.sprintf "invalid log level (%s)" str) |
There was a problem hiding this comment.
I'd say for this function, to have it evaluate to a level option. The Error result doesn't carry more information than None – there is only one way to fail, and the caller already knows which string they passed in. But to address your question, yes, we should use Result.result instead of Pervasives.result, in general.
There was a problem hiding this comment.
So, right now, shall we use option or introduce a dependency on the result compatibility library? If 4.02 support is a goal, option is the easiest way out, and I agree Error doesn't offer much more than None here, but I'll do whichever you tell me to do.
There was a problem hiding this comment.
I prefer option here :) But we already have the dependency on result (see lwt.opam).
There was a problem hiding this comment.
Oh, I missed it. Ok, I'll go with option then.
| | Some l -> rules := loop l | ||
| | None -> | ||
| if fail_on_error then raise (Failure "Invalid log rules") | ||
| else Printf.eprintf "Invalid contents of the LWT_LOG variable\n%!" |
There was a problem hiding this comment.
Since we're fixing this up anyway, I think the reference to LWT_LOG should be removed – this code can be called when not reading LWT_LOG. Alternatively, one could pass some kind of from_lwt_log parameter to control what this message looks like... but it may be kind of unnecessary. Ideally we would push the generation of the message to the caller, but that would require a breaking change :/
There was a problem hiding this comment.
I was just trying to keep the old behaviour completely intact, reference to LWT_LOG does bother me too of course. Shall we just replace the message with "Invalid contents of log rules" or similar?
There was a problem hiding this comment.
Up to you. Using the same message for both the exception and the log message is fine. I hope nobody was parsing their log for the string LWT_LOG :)
There was a problem hiding this comment.
That was my thought exactly... ;)
| val load_rules : string -> unit | ||
| val level_of_string : string -> (level, string) result | ||
|
|
||
| val load_rules : ?fail_on_error:bool -> string -> unit |
There was a problem hiding this comment.
Should probably document ?fail_on_error as, besides the exception, there is the non-trivial difference that load_rules ~fail_on_error:false will actually load as many rules as it can, while load_rules ~fail_on_error:true will load nothing if any error is encountered.
There was a problem hiding this comment.
Agree. I'll add a docstring.
Make level_of_string : string -> level option instead of (level, string) result Remove references to LWT_LOG env variable from messages.
|
Thank you! |
When set to true, ?fail_on_error causes load_rules raise Failure if it fails to parse
the rules, otherwise the behaviour remains the same for backwards compatibility.
The code that converts string representation of the level to Lwt_log_core.level was moved to a separate level_of_string : string -> (level, string) result function that is also exposed in the interface, it can be especially handy when the level comes from config or command line options, and one wants to use it in add_rule or other functions that take level.
On a side note, (String.lowercase [@ocaml.warning "-3"]) bothers me. Maybe change it to String.lowercase_ascii rather than supress the warning?