feat: add log filter #98
Conversation
cli/src/main.rs
Outdated
| @@ -8,8 +8,10 @@ mod compiler; | |||
| mod log_id; | |||
|
|
|||
| fn main() { | |||
| let log_handler = LogEventHandlerBuilder::new() | |||
| .write_to_console() | |||
| let _ = logid::set_filter!("info(infos)"); | |||
There was a problem hiding this comment.
May I ask why is this a macro? Looking at the source code I feel like it could (and should) be a function. Also, would it be possible to use enums for filter level? Using string feels very error prone to be honest.
There was a problem hiding this comment.
The idea was to have an environmental variable to set the filter, but the variable would need to be set during compiletime, and I was not able to get this to work using environmental variables in cargo's config.toml file.
With the string approach, setting the filter is the same for the environmental variable or via macro/function.
I also just wanted to get the filter working to have at least some filter.
The macro could be a function, but this would be a change in logid, and this might change in the future if I add another way to set the filter without using the string syntax.
There was a problem hiding this comment.
The idea was to have an environmental variable to set the filter, but the variable would need to be set during compiletime
Is there a reason you need the environment variable at the compile time? I would expect that to be something we can change after the compilation (at runtime). For example, if user reports an issue, we can instruct them to set the corresponding environment variable so that the compiler reports more information that can be useful for us.
The macro could be a function, but this would be a change in logid, and this might change in the future
Justification like this one is often problematic, because we don't know when that future will be. I think that if we can improve it now, we should do it now. The future change might be very soon, but it might also be next year, or maybe even never.
That being said, even if the macro signature will change (e.g. accepts type instead of literal), then it could still be a function. And if there should be other ways to construct a filter, I would prefer multiple functions over overloading a single macro.
Also, Macro heavy code is often harder to understand and increases compilation time. If we don't need features of macro (e.g. code to expand, or the function to be variadic or use custom syntax etc.), then it could probably be just a regular function. This looks like a candidate for me that should be reduced from macro to a function.
There was a problem hiding this comment.
Is there a reason you need the environment variable at the compile time? I would expect that to be something we can change after the compilation (at runtime). For example, if user reports an issue, we can instruct them to set the corresponding environment variable so that the compiler reports more information that can be useful for us.
It is needed at compilation time, because the logger is setup at compilation time, and so is the initial filter.
As I do not want to check if the environmental variable changed on every log entry, the environmental variable must be available at compile time to be used for the initial filter.
Justification like this one is often problematic, because we don't know when that future will be. I think that if we can improve it now, we should do it now. The future change might be very soon, but it might also be next year, or maybe even never.
Even if the change might not come in the next months, it only affects one line in Unimarkup, so I prefer to keep it as is for now, and use logid more to better understand what we need for filtering.
There was a problem hiding this comment.
changed to newest logid version that offers a filter builder instead of the set_filter macro.
filter builder was implemented by @nfejzic. Thank you :)
This PR uses the newest logid version to get log filtering.
The filter chosen is
info(infos), meaning all log levels ofinfoor higher, andinfoaddons.The newest logid version also offers macros to set addons more easily, but existing logs were not changed so far.