From ca0c5deaf5b9515a92ab54414debca2938c81107 Mon Sep 17 00:00:00 2001 From: Juan Facorro Date: Fri, 12 Sep 2014 16:43:15 -0300 Subject: [PATCH 1/4] [#107] Encapsulated access to configuration fields. Improved error messages for invalid config. --- src/elvis.erl | 30 ++++++++++----------- src/elvis_config.erl | 62 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 17 deletions(-) diff --git a/src/elvis.erl b/src/elvis.erl index 554185c..b75e3b9 100644 --- a/src/elvis.erl +++ b/src/elvis.erl @@ -48,11 +48,17 @@ rock() -> rock(Config). -spec rock(elvis_config:config()) -> ok | {fail, [elvis_result:file()]}. -rock(Config) when is_list(Config) -> - Results = lists:map(fun rock/1, Config), - lists:foldl(fun combine_results/2, ok, Results); -rock(Config = #{files := Files, rules := _Rules}) -> +rock(Config) -> + elvis_config:validate(Config), + NewConfig = elvis_config:normalize(Config), + Results = lists:map(fun do_rock/1, NewConfig), + lists:foldl(fun combine_results/2, ok, Results). + +%% @private +do_rock(Config0) -> elvis_utils:info("Loading files...~n"), + Config = elvis_config:resolve_files(Config0), + Files = elvis_config:files(Config), Fun = fun (File) -> Path = elvis_file:path(File), elvis_utils:info("Loading ~s~n", [Path]), @@ -66,18 +72,7 @@ rock(Config = #{files := Files, rules := _Rules}) -> case elvis_result:status(Results) of fail -> {fail, Results}; ok -> ok - end; -rock(Config = #{dirs := _Dirs, rules := _Rules}) -> - NewConfig = elvis_config:resolve_files(Config), - rock(NewConfig); -rock(Config = #{src_dirs := Dirs}) -> - %% NOTE: Provided for backwards compatibility. - %% Rename 'src_dirs' key to 'dirs'. - Config1 = maps:remove(src_dirs, Config), - Config2 = Config1#{dirs => Dirs}, - rock(Config2); -rock(Config) -> - throw({invalid_config, Config}). + end. %%% Git-Hook Command @@ -120,7 +115,8 @@ combine_results({fail, ItemResults}, {fail, AccResults}) -> -spec apply_rules(elvis_config:config(), elvis_file:file()) -> elvis_result:file(). -apply_rules(Config = #{rules := Rules}, File) -> +apply_rules(Config, File) -> + Rules = elvis_config:rules(Config), Acc = {[], Config, File}, {RulesResults, _, _} = lists:foldl(fun apply_rule/2, Acc, Rules), diff --git a/src/elvis_config.erl b/src/elvis_config.erl index 3531076..9be8e05 100644 --- a/src/elvis_config.erl +++ b/src/elvis_config.erl @@ -4,7 +4,13 @@ default/0, load_file/1, load/1, + validate/1, + normalize/1, + %% Geters dirs/1, + files/1, + rules/1, + %% Files resolve_files/1, resolve_files/2, apply_to_files/2 @@ -55,6 +61,38 @@ ensure_config_list(Config) when is_map(Config) -> ensure_config_list(Config) -> Config. +-spec validate(config()) -> ok | {error, term()}. +validate(Config) when is_list(Config) -> + lists:foreach(fun validate/1, Config); +validate(RuleGroup) -> + case maps:is_key(src_dirs, RuleGroup) or maps:is_key(dirs, RuleGroup) of + false -> throw({invalid_config, {missing_dirs, RuleGroup}}); + true -> ok + end, + case maps:is_key(dirs, RuleGroup) of + true -> + case maps:is_key(filter, RuleGroup) of + false -> throw({invalid_config, {missing_filter, RuleGroup}}); + true -> ok + end; + false -> ok + end, + case maps:is_key(rules, RuleGroup) of + false -> throw({invalid_config, {missing_rules, RuleGroup}}); + true -> ok + end. + +-spec normalize(config()) -> config(). +normalize(Config) when is_list(Config) -> + lists:map(fun normalize/1, Config); +normalize(Config = #{src_dirs := Dirs}) -> + %% NOTE: Provided for backwards compatibility. + %% Rename 'src_dirs' key to 'dirs'. + Config1 = maps:remove(src_dirs, Config), + Config1#{dirs => Dirs}; +normalize(Config) -> + Config. + -spec dirs(config()) -> [string()]. dirs(Config) when is_list(Config) -> lists:flatmap(fun dirs/1, Config); @@ -63,6 +101,22 @@ dirs(_RuleGroup = #{dirs := Dirs}) -> dirs(#{}) -> []. +-spec files(config()) -> [string()]. +files(_RuleGroup = #{files := Files}) -> + Files; +files(#{}) -> + undefined. + +-spec rules(config()) -> [string()]. +rules(_RuleGroup = #{rules := Rules}) -> + Rules; +rules(#{}) -> + undefined. + +%% @doc Takes a configuration and a list of files, filtering some +%% of them according to the 'filter' key, or if not specified +%% uses '*.erl'. +%% @end -spec resolve_files(config(), [elvis_file:file()]) -> config(). resolve_files(Config, Files) when is_list(Config) -> Fun = fun(RuleGroup) -> resolve_files(RuleGroup, Files) end, @@ -75,9 +129,14 @@ resolve_files(RuleGroup, Files) -> FilteredFiles = elvis_file:filter_files(Files, ?DEFAULT_FILTER), RuleGroup#{files => FilteredFiles}. +%% @doc Takes a configuration and finds all files according to its 'dirs' +%% end 'filter' key, or if not specified uses '*.erl'. +%% @end -spec resolve_files(config()) -> config(). resolve_files(Config) when is_list(Config) -> lists:map(fun resolve_files/1, Config); +resolve_files(RuleGroup = #{files := _Files}) -> + RuleGroup; resolve_files(RuleGroup = #{dirs := Dirs, filter := Filter}) -> Files = elvis_file:find_files(Dirs, Filter, local), RuleGroup#{files => Files}; @@ -85,6 +144,9 @@ resolve_files(RuleGroup = #{dirs := Dirs}) -> Files = elvis_file:find_files(Dirs, ?DEFAULT_FILTER), RuleGroup#{files => Files}. +%% @doc Takes a function and configuration and applies the function to all +%% file in the configuration. +%% @end -spec apply_to_files(fun(), config()) -> config(). apply_to_files(Fun, Config) when is_list(Config) -> ApplyFun = fun(RuleGroup) -> apply_to_files(Fun, RuleGroup) end, From d1e24d5e2a604f38c092d0d6fe68668c1ba9b739 Mon Sep 17 00:00:00 2001 From: Juan Facorro Date: Fri, 12 Sep 2014 18:58:23 -0300 Subject: [PATCH 2/4] [#107] Added color to output. --- src/elvis.erl | 10 ++++----- src/elvis_code.erl | 1 - src/elvis_config.erl | 2 ++ src/elvis_result.erl | 10 ++++----- src/elvis_utils.erl | 52 ++++++++++++++++++++++++++++++++++++++++++-- test/elvis_SUITE.erl | 16 +++++++++----- 6 files changed, 73 insertions(+), 18 deletions(-) diff --git a/src/elvis.erl b/src/elvis.erl index b75e3b9..f124842 100644 --- a/src/elvis.erl +++ b/src/elvis.erl @@ -36,7 +36,7 @@ main(Args) -> {ok, {Options, Commands}} -> process_options(Options, Commands); {error, {Reason, Data}} -> - io:format("Error: ~s ~p~n~n", [Reason, Data]), + elvis_utils:error_prn("~s ~p~n~n", [Reason, Data]), help() end. @@ -56,17 +56,17 @@ rock(Config) -> %% @private do_rock(Config0) -> - elvis_utils:info("Loading files...~n"), + elvis_utils:info("Loading files..."), Config = elvis_config:resolve_files(Config0), Files = elvis_config:files(Config), Fun = fun (File) -> Path = elvis_file:path(File), - elvis_utils:info("Loading ~s~n", [Path]), + elvis_utils:info("Loading ~s", [Path]), elvis_file:load_file_data(Config, File) end, LoadedFiles = lists:map(Fun, Files), - elvis_utils:info("Applying rules...~n"), + elvis_utils:info("Applying rules..."), Results = [apply_rules(Config, File) || File <- LoadedFiles], case elvis_result:status(Results) of @@ -151,7 +151,7 @@ process_options(Options, Commands) -> process_options(Options, AtomCommands, Config) catch throw:Exception -> - io:format("Error: ~p.~n", [Exception]) + elvis_utils:error_prn("~p.", [Exception]) end. -spec process_options([atom()], [string()], elvis_config:config()) -> ok. diff --git a/src/elvis_code.erl b/src/elvis_code.erl index 898d4e9..bb800a7 100644 --- a/src/elvis_code.erl +++ b/src/elvis_code.erl @@ -734,7 +734,6 @@ to_map({type, Attrs, constraint, [Sub, SubType]}) -> subtype => Sub}, content => to_map(SubType)}; to_map({type, Attrs, bounded_fun, [FunType, Defs]}) -> - %% io:format("~p~n", [Defs]), #{type => type, attrs => #{location => get_location(Attrs), text => get_text(Attrs), diff --git a/src/elvis_config.erl b/src/elvis_config.erl index 9be8e05..105a3d1 100644 --- a/src/elvis_config.erl +++ b/src/elvis_config.erl @@ -62,6 +62,8 @@ ensure_config_list(Config) -> Config. -spec validate(config()) -> ok | {error, term()}. +validate([]) -> + throw({invalid_config, empty_config}); validate(Config) when is_list(Config) -> lists:foreach(fun validate/1, Config); validate(RuleGroup) -> diff --git a/src/elvis_result.erl b/src/elvis_result.erl index a7a4c7a..fae7ac8 100644 --- a/src/elvis_result.erl +++ b/src/elvis_result.erl @@ -100,21 +100,21 @@ print([Result | Results]) -> print(#{file := File, rules := Rules}) -> Path = elvis_file:path(File), Status = case status(Rules) of - ok -> "OK"; - fail -> "FAIL" + ok -> "{{green-bold}}OK"; + fail -> "{{red-bold}}FAIL" end, - elvis_utils:info("# ~s [~s]~n", [Path, Status]), + elvis_utils:notice("# ~s [~s{{white-bold}}]", [Path, Status]), print(Rules); print(#{items := []}) -> ok; print(#{name := Name, items := Items}) -> - elvis_utils:info(" - ~s~n", [atom_to_list(Name)]), + elvis_utils:notice(" - ~s", [atom_to_list(Name)]), print(Items); print(#{message := Msg, info := Info}) -> - elvis_utils:info(" - " ++ Msg ++ "~n", Info). + elvis_utils:notice(" - " ++ Msg, Info). -spec status([rule()]) -> ok | fail. status([]) -> diff --git a/src/elvis_utils.erl b/src/elvis_utils.erl index 774710e..9d9c03a 100644 --- a/src/elvis_utils.erl +++ b/src/elvis_utils.erl @@ -14,7 +14,12 @@ %% Output info/1, - info/2 + info/2, + notice/1, + notice/2, + error_prn/1, + error_prn/2, + parse_colors/1 ]). -export_type([file/0]). @@ -130,7 +135,50 @@ info(Message) -> -spec info(string(), [term()]) -> ok. info(Message, Args) -> + ColoredMessage = Message ++ "{{reset}}~n", + print(ColoredMessage, Args). + +-spec notice(string()) -> ok. +notice(Message) -> + notice(Message, []). + +-spec notice(string(), [term()]) -> ok. +notice(Message, Args) -> + ColoredMessage = "{{white-bold}}" ++ Message ++ "{{reset}}~n", + print(ColoredMessage, Args). + + +-spec error_prn(string()) -> ok. +error_prn(Message) -> + error_prn(Message, []). + +-spec error_prn(string(), [term()]) -> ok. +error_prn(Message, Args) -> + ColoredMessage = "{{red}}Error: {{reset}}" ++ Message ++ "{{reset}}~n", + print(ColoredMessage, Args). + +print(Message, Args) -> case application:get_env(elvis, no_output) of {ok, true} -> ok; - _ -> io:format(Message, Args) + _ -> + Output = io_lib:format(Message, Args), + io:format(parse_colors(Output)) end. + + +-spec parse_colors(string()) -> string(). +parse_colors(Message) -> + Colors = #{"red" => "\e[0;31m", + "red-bold" => "\e[1;31m", + "green" => "\e[0;32m", + "green-bold" => "\e[1;32m", + "white" => "\e[0;37m", + "white-bold" => "\e[1;37m", + "reset" => "\e[0m"}, + Opts = [global, {return, list}], + Fun = fun(Key, Acc) -> + Regex = ["{{", Key, "}}"], + Color = maps:get(Key, Colors), + re:replace(Acc, Regex, Color, Opts) + end, + lists:foldl(Fun, Message, maps:keys(Colors)). diff --git a/test/elvis_SUITE.erl b/test/elvis_SUITE.erl index 0ff79f3..4f2b367 100644 --- a/test/elvis_SUITE.erl +++ b/test/elvis_SUITE.erl @@ -77,6 +77,12 @@ rock_with_empty_map_config(_Config) -> fail catch throw:{invalid_config, _} -> ok + end, + ok = try + elvis:rock([]), + fail + catch + throw:{invalid_config, _} -> ok end. -spec rock_with_empty_list_config(config()) -> any(). @@ -305,7 +311,7 @@ main_git_hook_fail(_Config) -> catch meck:unload(elvis_utils), meck:unload(elvis_git) - end. + end. -spec main_git_hook_ok(config()) -> any(). main_git_hook_ok(_Config) -> @@ -357,7 +363,7 @@ check_some_line_output(Fun, Expected, FilterFun) -> check_first_line_output(Fun, Expected) -> Equals = fun(Result, Exp) -> - Result == Exp + Result == Exp end, check_first_line_output(Fun, Expected, Equals). @@ -366,9 +372,9 @@ check_first_line_output(Fun, Expected, FilterFun) -> Fun(), ct:capture_stop(), Lines = case ct:capture_get([]) of - [] -> []; - [Head | _] -> [Head] - end, + [] -> []; + [Head | _] -> [Head] + end, ListFun = fun(Line) -> FilterFun(Line, Expected) end, [_ | _] = lists:filter(ListFun, Lines). From 01a0a4337dcb104468495629c1fb70870108addd Mon Sep 17 00:00:00 2001 From: Juan Facorro Date: Mon, 15 Sep 2014 11:34:17 -0300 Subject: [PATCH 3/4] [#107] Updated tests that check console output. --- Makefile | 2 +- src/elvis.erl | 15 +++++++++++---- test/elvis_SUITE.erl | 28 ++++++++++++++-------------- 3 files changed, 26 insertions(+), 19 deletions(-) diff --git a/Makefile b/Makefile index a1feec5..9f81e90 100644 --- a/Makefile +++ b/Makefile @@ -22,7 +22,7 @@ ERLC_OPTS += +warn_export_vars +warn_exported_vars +warn_missing_spec +warn_unty # Commont Test Config TEST_ERLC_OPTS += +'{parse_transform, lager_transform}' -CT_SUITES = project elvis style git +CT_SUITES = elvis style project git CT_OPTS = -cover test/elvis.coverspec -erl_args -config config/test.config # Builds the elvis escript. diff --git a/src/elvis.erl b/src/elvis.erl index f124842..5ed72d7 100644 --- a/src/elvis.erl +++ b/src/elvis.erl @@ -124,11 +124,18 @@ apply_rules(Config, File) -> elvis_result:print(Results), Results. -apply_rule({Module, Function, Args}, {Result, Config, File}) -> - Results = Module:Function(Config, File, Args), - RuleResult = elvis_result:new(rule, Function, Results), +apply_rule({Module, Function, Args}, Acc = {Result, Config, File}) -> + try + Results = Module:Function(Config, File, Args), + RuleResult = elvis_result:new(rule, Function, Results), - {[RuleResult | Result], Config, File}. + {[RuleResult | Result], Config, File} + catch + _:Reason -> + Msg = "~p while applying rule '~p'", + elvis_utils:error_prn(Msg, [Function, Reason]), + Acc + end. %%% Command Line Interface diff --git a/test/elvis_SUITE.erl b/test/elvis_SUITE.erl index 4f2b367..d41b324 100644 --- a/test/elvis_SUITE.erl +++ b/test/elvis_SUITE.erl @@ -121,7 +121,7 @@ rock_with_list_config(_Config) -> -spec rock_with_file_config(config()) -> ok. rock_with_file_config(_Config) -> Fun = fun() -> elvis:rock() end, - Expected = "# \\.\\./\\.\\./test/examples/.*\\.erl \\[FAIL\\]\n", + Expected = "# \\.\\./\\.\\./test/examples/.*\\.erl.*FAIL", check_some_line_output(Fun, Expected, fun matches_regex/2), ok. @@ -250,14 +250,14 @@ main_commands(_Config) -> -spec main_config(config()) -> any(). main_config(_Config) -> - Expected = "Error: missing_option_arg config", + Expected = "missing_option_arg config", OptFun = fun() -> elvis:main("-c") end, - check_first_line_output(OptFun, Expected, fun starts_with/2), + check_first_line_output(OptFun, Expected, fun matches_regex/2), - EnoentExpected = "Error: enoent.\n", + EnoentExpected = "enoent", OptEnoentFun = fun() -> elvis:main("-c missing") end, - check_first_line_output(OptEnoentFun, EnoentExpected), + check_first_line_output(OptEnoentFun, EnoentExpected, fun matches_regex/2), ConfigFun = fun() -> elvis:main("-c ../../config/elvis.config") end, check_emtpy_output(ConfigFun), @@ -265,17 +265,17 @@ main_config(_Config) -> -spec main_rock(config()) -> any(). main_rock(_Config) -> - ExpectedFail = "# \\.\\./\\.\\./test/examples/.*\\.erl \\[FAIL\\]\n", + ExpectedFail = "# \\.\\./\\.\\./test/examples/.*\\.erl.*FAIL", NoConfigArgs = "rock", NoConfigFun = fun() -> elvis:main(NoConfigArgs) end, check_some_line_output(NoConfigFun, ExpectedFail, fun matches_regex/2), - Expected = "# ../../src/elvis.erl [OK]", + Expected = "# ../../src/elvis.erl.*OK", ConfigArgs = "rock -c ../../config/elvis-test.config", ConfigFun = fun() -> elvis:main(ConfigArgs) end, - check_some_line_output(ConfigFun, Expected, fun starts_with/2), + check_some_line_output(ConfigFun, Expected, fun matches_regex/2), ok. @@ -299,11 +299,11 @@ main_git_hook_fail(_Config) -> FakeStagedFiles = fun() -> Files end, meck:expect(elvis_git, staged_files, FakeStagedFiles), - Expected = "# fake_long_line.erl [FAIL]", + Expected = "# fake_long_line.erl.*FAIL", ConfigArgs = "git-hook -c ../../config/elvis-test.config", ConfigFun = fun() -> elvis:main(ConfigArgs) end, - check_some_line_output(ConfigFun, Expected, fun starts_with/2), + check_some_line_output(ConfigFun, Expected, fun matches_regex/2), meck:expect(elvis_git, staged_files, fun() -> [] end), check_emtpy_output(ConfigFun) @@ -332,9 +332,9 @@ main_default_config(_Config) -> Dest = "./elvis.config", file:copy(Src, Dest), - Expected = "# ../../src/elvis.erl [OK]", + Expected = "# ../../src/elvis.erl.*OK", RockFun = fun() -> elvis:main("rock") end, - check_some_line_output(RockFun, Expected, fun starts_with/2), + check_some_line_output(RockFun, Expected, fun matches_regex/2), file:delete(Dest), @@ -342,10 +342,10 @@ main_default_config(_Config) -> -spec main_unexistent(config()) -> any(). main_unexistent(_Config) -> - Expected = "Error: unrecognized_or_unimplemened_command.\n", + Expected = "unrecognized_or_unimplemened_command.", UnexistentFun = fun() -> elvis:main("aaarrrghh") end, - check_first_line_output(UnexistentFun, Expected), + check_first_line_output(UnexistentFun, Expected,fun matches_regex/2), ok. From 9eb8061e1d41da217ea3626fed608f825776ff0d Mon Sep 17 00:00:00 2001 From: Juan Facorro Date: Mon, 15 Sep 2014 13:40:44 -0300 Subject: [PATCH 4/4] [#107] Improved error messages on file loading and rule applying. --- src/elvis.erl | 45 ++++++++++++++++++++++++++------------------ src/elvis_result.erl | 13 +++++++++---- 2 files changed, 36 insertions(+), 22 deletions(-) diff --git a/src/elvis.erl b/src/elvis.erl index 5ed72d7..35b6c72 100644 --- a/src/elvis.erl +++ b/src/elvis.erl @@ -55,17 +55,13 @@ rock(Config) -> lists:foldl(fun combine_results/2, ok, Results). %% @private +-spec do_rock(elvis_config:config()) -> ok | {fail, [elvis_result:file()]}. do_rock(Config0) -> elvis_utils:info("Loading files..."), Config = elvis_config:resolve_files(Config0), Files = elvis_config:files(Config), - Fun = fun (File) -> - Path = elvis_file:path(File), - elvis_utils:info("Loading ~s", [Path]), - elvis_file:load_file_data(Config, File) - end, + Fun = fun (File) -> load_file_data(Config, File) end, LoadedFiles = lists:map(Fun, Files), - elvis_utils:info("Applying rules..."), Results = [apply_rules(Config, File) || File <- LoadedFiles], @@ -74,6 +70,21 @@ do_rock(Config0) -> ok -> ok end. +%% @private +-spec load_file_data(elvis_config:config(), elvis_file:file()) -> + elvis_file:file(). +load_file_data(Config, File) -> + Path = elvis_file:path(File), + elvis_utils:info("Loading ~s", [Path]), + try + elvis_file:load_file_data(Config, File) + catch + _:Reason -> + Msg = "~p when loading file ~p.", + elvis_utils:error_prn(Msg, [Reason, Path]), + File + end. + %%% Git-Hook Command -spec git_hook(elvis_config:config()) -> ok. @@ -124,18 +135,16 @@ apply_rules(Config, File) -> elvis_result:print(Results), Results. -apply_rule({Module, Function, Args}, Acc = {Result, Config, File}) -> - try - Results = Module:Function(Config, File, Args), - RuleResult = elvis_result:new(rule, Function, Results), - - {[RuleResult | Result], Config, File} - catch - _:Reason -> - Msg = "~p while applying rule '~p'", - elvis_utils:error_prn(Msg, [Function, Reason]), - Acc - end. +apply_rule({Module, Function, Args}, {Result, Config, File}) -> + RuleResult = try + Results = Module:Function(Config, File, Args), + elvis_result:new(rule, Function, Results) + catch + _:Reason -> + Msg = "'~p' while applying rule '~p'.", + elvis_result:new(error, Msg, [Reason, Function]) + end, + {[RuleResult | Result], Config, File}. %%% Command Line Interface diff --git a/src/elvis_result.erl b/src/elvis_result.erl index fae7ac8..569e0e1 100644 --- a/src/elvis_result.erl +++ b/src/elvis_result.erl @@ -58,7 +58,9 @@ new(item, Msg, Info) -> new(rule, Name, Results) -> #{name => Name, items => Results}; new(file, File, Rules) -> - #{file => File, rules => Rules}. + #{file => File, rules => Rules}; +new(error, Msg, Info) -> + #{error_msg => Msg, info => Info}. -spec new(item, term(), any(), any()) -> item(). new(item, Msg, Info, LineNum) -> @@ -106,15 +108,18 @@ print(#{file := File, rules := Rules}) -> elvis_utils:notice("# ~s [~s{{white-bold}}]", [Path, Status]), print(Rules); - +%% Rule print(#{items := []}) -> ok; print(#{name := Name, items := Items}) -> elvis_utils:notice(" - ~s", [atom_to_list(Name)]), print(Items); - +%% Item print(#{message := Msg, info := Info}) -> - elvis_utils:notice(" - " ++ Msg, Info). + elvis_utils:notice(" - " ++ Msg, Info); +%% Error +print(#{error_msg := Msg, info := Info}) -> + elvis_utils:error_prn(Msg, Info). -spec status([rule()]) -> ok | fail. status([]) ->