Skip to content
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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 20, 2024
+17 −2
Merged

Make log_level entry in rebar.app work again #2860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion apps/rebar/src/rebar.app.src.script
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
]},
{env, [
%% Default log level
{log_level, warn},
{log_level, info},

{resources, [{git, rebar_git_resource},
{git_subdir, rebar_git_subdir_resource},
Expand Down
7 changes: 6 additions & 1 deletion apps/rebar/src/rebar3.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,12 @@ log_level() ->
Di when Di == false; Di == "" ->
case os:getenv("DEBUG") of
D when D == false; D == "" ->
rebar_log:default_level();
try
{ok, L} = application:get_env(rebar, log_level),
rebar_log:atom_to_level(L)
catch
_:_ -> rebar_log:default_level()
end;
Comment on lines +309 to +314
Copy link
Contributor

@vkatsuba vkatsuba Jan 30, 2024 •
edited
Loading

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:

Suggested change
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));

Copy link
Contributor Author

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.

Copy link
Contributor

@vkatsuba vkatsuba Jan 30, 2024 •
edited
Loading

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

_ ->
rebar_log:debug_level()
end;
Expand Down
10 changes: 10 additions & 0 deletions apps/rebar/src/rebar_log.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
default_level/0,
debug_level/0,
diagnostic_level/0,
atom_to_level/1,
intensity/0,
log/3,
is_verbose/1,
Expand Down Expand Up @@ -143,6 +144,15 @@ is_verbose(State) ->
valid_level(Level) ->
erlang:max(?ERROR_LEVEL, erlang:min(Level, ?DIAGNOSTIC_LEVEL)).

atom_to_level(Level) ->
case Level of
error -> ?ERROR_LEVEL;
warn -> ?WARN_LEVEL;
info -> ?INFO_LEVEL;
debug -> ?DEBUG_LEVEL;
diagnostic -> ?DIAGNOSTIC_LEVEL
end.

%% ===================================================================
%% Internal functions
%% ===================================================================
Expand Down
Loading