Match all submodules on with_module_level
#27
Conversation
Added a test, just to be sure. Also some examples were missing a `unwrap()` after calling `init()`, fixed that too. Closes borntyping#24.
There was a problem hiding this comment.
Hello @piegamesde !
I have reviewed your PR, and have some little questions!
Regarding with_target_levels I don't think we should keep the method. It was added on my first commit f02c401 before the builder pattern was available. Before that, one needed to initialize the logger in one time, that's why the method does overwrite the level filters.
When I have implemented the builder pattern, I have simply move the method without thinking about the implications (😅), even the name target does not make sense now.
I would simply remove the method since people should use with_module_level instead. I know it should raise major version as it is a breaking change, but If we take time to perform reverse dependency analysis, I think nobody use this method anyway. Raising simply the minor version should be sufficient, as it will cause a compile time error for the caller, so nothing will be hidden.
What do you think @borntyping @piegamesde ?
|
Thanks for the review! I'll deprecate |
Great! I've just had a little suggestion regarding the module ordering implementation, otherwise everything LGTM! Thanks for the feature, this will definitely be helpful :) |
|
Hello there @borntyping @piegamesde! Any news on when this will be merged? :) |
|
Just waiting on CI checks to all pass. |
|
Passed with a retry :) |
Added a test, just to be sure.
Also some examples were missing a
unwrap()after callinginit(), fixed that too.Closes #24.
I'm unsure about
with_target_levels, since that takes aHashMapand we don't use that anymore (for now). Changing this is a bit tricky due to backwards compatibility. I think I could modify this into something that take animpl IntoIterator<(String, LevelFilter)>or the like so that the actual data structure is hidden as an implementation detail. Also I'm not sure about the semantics of that method, why does it overwrite all existing level filters instead of adding them? Can I change this or is this considered a breaking change? (@creekorful you wrote this, any opinions?)