-
Notifications
You must be signed in to change notification settings - Fork 518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make log_level entry in rebar.app work again #2860
Conversation
The log_level app setting had become ignored during refactorings of old. This makes it take effect again and syncs its standard setting with that of with rebar_log:default_level(). We add rebar_log:atom_to_level/1 to avoid exposing the level numbers used internally.
try | ||
{ok, L} = application:get_env(rebar, log_level), | ||
rebar_log:atom_to_level(L) | ||
catch | ||
_:_ -> rebar_log:default_level() | ||
end; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like that it can be simplify like:
try | |
{ok, L} = application:get_env(rebar, log_level), | |
rebar_log:atom_to_level(L) | |
catch | |
_:_ -> rebar_log:default_level() | |
end; | |
rebar_log:atom_to_level(application:get_env(rebar, log_level, info)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like that it can be simplify like:
No, even if you call get_env(rebar, log_level, rebar_log:default_level()) so that this code does not hardcode the info value, someone may still put an invalid value in the configuration, and in that case I also want to substitute the default rather than failing to start.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this case you can change logic in to_level function:
atom_to_level(Level) ->
case Level of
error -> ?ERROR_LEVEL;
warn -> ?WARN_LEVEL;
debug -> ?DEBUG_LEVEL;
diagnostic -> ?DIAGNOSTIC_LEVEL;
_ -> ?INFO_LEVEL
end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but that is also not appropriate. The conversion function should work on defined levels only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you doing totally same in catch block - if set incorrect value and it will crashed over missing matching - then use default value - default value is ?INFO_LEVEL
based on the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But you doing totally same in catch block - if set incorrect value and it will crashed over missing matching - then use default value - default value is
?INFO_LEVEL
based on the code.
Those modules and functions have very different areas of responsibility.
I think that was meant to be removed from Not that this isn't an ok change since its not like it hurts to have I guess. |
Yeah, I thought about dropping it, but it's a nice and easy way to rebuild rebar with your default level of choice. Also, the only other way to change the level currently is by setting env variables like DEBUG or QUIET. |
The log_level app setting had become ignored during refactorings of old. This makes it take effect again and syncs its standard setting with that of with rebar_log:default_level(). We add rebar_log:atom_to_level/1 to avoid exposing the level numbers used internally.