[Closes #181] Operator spaces bug

Makefile

@@ -3,16 +3,17 @@ PROJECT = elvis
 DEPS = lager sync getopt jiffy ibrowse aleppo zipper egithub katana
 TEST_DEPS = meck
 
-dep_lager = git https://github.com/basho/lager.git 2.0.3
-dep_sync = git https://github.com/rustyio/sync.git master
-dep_getopt = git https://github.com/jcomellas/getopt v0.8.2
-dep_meck = git https://github.com/eproxus/meck 0.8.2
-dep_jiffy = git https://github.com/davisp/jiffy 0.11.3
-dep_ibrowse = git https://github.com/cmullaparthi/ibrowse v4.1.1
-dep_aleppo = git https://github.com/inaka/aleppo 0.9.1
-dep_zipper = git https://github.com/inaka/zipper 0.1.0
-dep_egithub = git https://github.com/inaka/erlang-github 0.1.7
-dep_katana =  git https://github.com/inaka/erlang-katana 0.2.2
+dep_lager = git git://github.com/basho/lager.git 2.0.3
+dep_sync = git git://github.com/inaka/sync.git 0.1.3
+dep_getopt = git git://github.com/jcomellas/getopt v0.8.2
+dep_meck = git git://github.com/eproxus/meck 0.8.2
+dep_jiffy = git git://github.com/davisp/jiffy 0.11.3
+dep_ibrowse = git git://github.com/cmullaparthi/ibrowse v4.1.1
+dep_aleppo = git git://github.com/inaka/aleppo 0.9.1
+dep_zipper = git git://github.com/inaka/zipper 0.1.0
+dep_egithub = git git://github.com/inaka/erlang-github 0.1.7
+dep_katana =  git git://github.com/inaka/erlang-katana 0.2.3
+
 
 include erlang.mk
 
@@ -24,7 +25,6 @@ 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 = elvis style project git
 CT_OPTS = -cover test/elvis.coverspec  -erl_args -config config/test.config
 
 # Builds the elvis escript.
@@ -40,3 +40,12 @@ test-shell: build-ct-suites app
 
 install: escript
 	cp elvis /usr/local/bin
+
+quicktests: ERLC_OPTS = $(TEST_ERLC_OPTS)
+quicktests: clean app build-ct-suites
+	@if [ -d "test" ] ; \
+	then \
+		mkdir -p logs/ ; \
+		$(CT_RUN) -suite $(addsuffix _SUITE,$(CT_SUITES)) $(CT_OPTS) ; \
+	fi
+	$(gen_verbose) rm -f test/*.beam

src/elvis_code.erl

@@ -3,7 +3,7 @@
 %% General
 -export([
          find/2,
-         find_zipper/2,
+         find/3,
          find_by_location/2
         ]).
 
@@ -20,71 +20,105 @@
 %%% Public API
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 
-%% @doc Finds all nodes that comply with the predicate function.
--spec find(fun((ktn_code:tree_node()) ->
-    boolean()), ktn_code:tree_node()) -> [ktn_code:tree_node()].
-find(Pred, Node) ->
-    Results = find(Pred, Node, []),
-    lists:flatten(Results).
-
-find(_Pred, [], Results) ->
-    Results;
-find(Pred, [Node | Nodes], Results) ->
-    [find(Pred, Node, Results) | find(Pred, Nodes, [])];
-find(Pred, Node, Results) ->
-    Content = ktn_code:content(Node),
-    case Pred(Node) of
-        true ->
-            find(Pred, Content, [Node | Results]);
-        false ->
-            find(Pred, Content, Results)
-    end.
+-type find_options() :: #{mode => node | zipper,
+                          traverse => content | all}.
 
--spec find_zipper(fun((zipper:zipper()) -> boolean()), ktn_code:tree_node()) ->
+%% @doc Same as calling find/3 with `#{mode => node, traverse => content}` as
+%%      the options map.
+%% @end
+-spec find(fun((zipper:zipper()) -> boolean()),
+           ktn_code:tree_node()) ->
     [ktn_code:tree_node()].
-find_zipper(Pred, Root) ->
+find(Pred, Root) ->
+    find(Pred, Root, #{mode => node, traverse => content}).
+
+%% @doc Find all nodes in the tree for which the predicate function returns
+%%      `true`. The options map has two keys:
+%%        - `mode`: when the value `node` is specified the predicate function
+%%          receives a tree_node() as its argument. When `zipper` is specified
+%%          the argument is the zipper location for the current node.
+%%        - `traverse`: the value `content` indicates to only take into account
+%%          nodes in the parent-child hierarchy. When `all` is provided the
+%%          nodes held in the `node_attrs` map are also taken into account in
+%%          the search.
+%% @end
+-spec find(fun((zipper:zipper()) -> boolean()),
+           ktn_code:tree_node(),
+           find_options()) ->
+    [ktn_code:tree_node()].
+find(Pred, Root, Opts) ->
+    Mode = ktn_maps:get(mode, Opts, node),
+    ZipperMode = ktn_maps:get(traverse, Opts, content),
+
+    Zipper = case ZipperMode of
+                 content -> content_zipper(Root);
+                 all -> all_zipper(Root)
+             end,
+    Results = find(Pred, Zipper, [], Mode),
+    lists:reverse(Results).
+
+-spec content_zipper(ktn_code:tree_node()) -> zipper:zipper().
+content_zipper(Root) ->
     IsBranch = fun
                    (#{content := [_ | _]}) -> true;
                    (_) -> false
                end,
     Children = fun (#{content := Content}) -> Content end,
     MakeNode = fun(Node, _) -> Node end,
-    Zipper = zipper:new(IsBranch, Children, MakeNode, Root),
-    Results = find_zipper(Pred, Zipper, []),
-    lists:reverse(Results).
+    zipper:new(IsBranch, Children, MakeNode, Root).
 
-find_zipper(Pred, Zipper, Results) ->
+-spec all_zipper(ktn_code:tree_node()) -> zipper:zipper().
+all_zipper(Root) ->
+    IsBranch = fun (Node = #{}) ->
+                   ktn_code:content(Node) =/= []
+                   orelse maps:is_key(node_attrs, Node)
+               end,
+    Children = fun
+                   (#{content := Content, node_attrs := NodeAttrs}) ->
+                       Content ++ lists:flatten(maps:values(NodeAttrs));
+                   (#{node_attrs := NodeAttrs}) ->
+                       lists:flatten(maps:values(NodeAttrs));
+                   (#{content := Content}) ->
+                       Content
+               end,
+    MakeNode = fun(Node, _) -> Node end,
+    zipper:new(IsBranch, Children, MakeNode, Root).
+
+find(Pred, Zipper, Results, Mode) ->
     case zipper:is_end(Zipper) of
         true ->
             Results;
         false ->
-            Node = zipper:node(Zipper),
-            NewResults = case Pred(Zipper) of
-                             true -> [Node | Results];
+            Value = case Mode of
+                        zipper -> Zipper;
+                        node -> zipper:node(Zipper)
+                    end,
+            NewResults = case Pred(Value) of
+                             true -> [zipper:node(Zipper) | Results];
                              false -> Results
                          end,
-            find_zipper(Pred, zipper:next(Zipper), NewResults)
+            find(Pred, zipper:next(Zipper), NewResults, Mode)
     end.
 
-
 -spec find_by_location(ktn_code:tree_node(), {integer(), integer()}) ->
     not_found | {ok, ktn_code:tree_node()}.
-find_by_location(Root, {Line, Column}) ->
-    Fun = fun
-              (Node = #{attrs := #{location := {NodeLine, NodeCol}}})
-                when NodeLine == Line->
-                  Text = ktn_code:attr(text, Node),
-                  Length = length(Text),
-                  (NodeCol =< Column) and (Column =< NodeCol + Length);
-              (_) ->
-                  false
+find_by_location(Root, Location) ->
+    Fun = fun (Node) ->
+              is_at_location(Node, Location)
           end,
-    case find(Fun, Root) of
+    case find(Fun, Root, #{traverse => all}) of
         [] -> not_found;
-        [Node | _] ->
-            {ok, Node}
+        [Node | _] -> {ok, Node}
     end.
 
+is_at_location(Node = #{attrs := #{location := {Line, NodeCol}}},
+               {Line, Column}) ->
+    Text = ktn_code:attr(text, Node),
+    Length = length(Text),
+    (NodeCol =< Column) andalso (Column < NodeCol + Length);
+is_at_location(_, _) ->
+    false.
+
 %%% Processing functions
 
 %% @doc Takes a node and returns all nodes where the nesting limit is exceeded.

src/elvis_style.erl

@@ -86,7 +86,8 @@ line_length(_Config, Target, RuleConfig) ->
     Limit = maps:get(limit, RuleConfig, 80),
     SkipComments = maps:get(skip_comments, RuleConfig, false),
     {Src, _} = elvis_file:src(Target),
-    elvis_utils:check_lines(Src, fun check_line_length/3, [Limit, SkipComments]).
+    Args = [Limit, SkipComments],
+    elvis_utils:check_lines(Src, fun check_line_length/3, Args).
 
 -spec no_tabs(elvis_config:config(), elvis_file:file(), [term()]) ->
     [elvis_result:item()].
@@ -185,7 +186,7 @@ invalid_dynamic_call(Config, Target, RuleConfig) ->
 used_ignored_variable(Config, Target, _RuleConfig) ->
     {Root, _} = elvis_file:parse_tree(Config, Target),
     ResultFun = result_node_line_col_fun(?USED_IGNORED_VAR_MSG),
-    case elvis_code:find_zipper(fun is_ignored_var/1, Root) of
+    case elvis_code:find(fun is_ignored_var/1, Root, #{mode => zipper}) of
         [] ->
             [];
         UsedIgnoredVars ->
@@ -376,7 +377,7 @@ check_macro_names(Line, Num, _Args) ->
 
 -spec check_macro_module_names(binary(), integer(), [term()]) ->
     no_result | {ok, elvis_result:item_result()}.
-check_macro_module_names(Line, Num, Root) ->
+check_macro_module_names(Line, Num, [Root]) ->
     {ok, ModNameRegex} = re:compile("[?](\\w+)[:][?]?\\w+\\s*\\("),
     {ok, FunNameRegex} = re:compile("[?]?\\w+[:][?](\\w+)\\s*\\("),
 
@@ -409,7 +410,7 @@ apply_macro_module_names(Line, Num, Regex, Msg, Root) ->
             MacroName = binary_to_list(binary:part(Line, Col, Len)),
             case
                 lists:member(MacroName, ?MACRO_MODULE_NAMES_EXCEPTIONS)
-                or not is_remote_call({Num, Col + 1}, Root)
+                orelse not is_remote_call({Num, Col}, Root)
             of
                 true ->
                     [];
@@ -427,10 +428,10 @@ is_remote_call({Num, Col}, Root) ->
         {ok, Node0} ->
             Pred =
                 fun(Zipper) ->
-                        (Node0 == zipper:node(Zipper))
-                        andalso has_remote_call_parent(Zipper)
+                    (Node0 == zipper:node(Zipper))
+                    andalso has_remote_call_parent(Zipper)
                 end,
-            [] =/= elvis_code:find_zipper(Pred, Root)
+            [] =/= elvis_code:find(Pred, Root, #{mode => zipper})
     end.
 
 has_remote_call_parent(undefined) ->
@@ -474,6 +475,7 @@ check_operator_spaces_rule(Line, Num, {Position, Operator}, Root) ->
                 atom -> [];
                 binary_element -> [];
                 string -> [];
+                char -> [];
                 comment -> [];
                 _ ->
                     Msg = ?OPERATOR_SPACE_MSG,
@@ -515,10 +517,10 @@ check_invalid_dynamic_calls(Root) ->
 is_dynamic_call(Node) ->
     case ktn_code:type(Node) of
         call ->
-            FunctionSpec = ktn_code:attr(function, Node),
+            FunctionSpec = ktn_code:node_attr(function, Node),
             case ktn_code:type(FunctionSpec) of
                 remote ->
-                    ModuleName = ktn_code:attr(module, FunctionSpec),
+                    ModuleName = ktn_code:node_attr(module, FunctionSpec),
                     var == ktn_code:type(ModuleName);
                 _Other ->
                     false
@@ -560,7 +562,7 @@ is_otp_module(Root) ->
                              gen_fsm,
                              supervisor_bridge
                             ]),
-    IsBehaviorAttr = fun(Node) ->  behavior == ktn_code:type(Node) end,
+    IsBehaviorAttr = fun(Node) -> behavior == ktn_code:type(Node) end,
     case elvis_code:find(IsBehaviorAttr, Root) of
         [] ->
             false;

src/elvis_utils.erl

@@ -10,7 +10,6 @@
          %% General
          erlang_halt/1,
          to_str/1,
-         maps_get/3,
 
          %% Output
          info/1,
@@ -123,13 +122,6 @@ indentation(Line, Char, Count) ->
         _ -> invalid
     end.
 
--spec maps_get(term(), map(), term()) -> term().
-maps_get(Key, Map, Default) ->
-    case maps:is_key(Key, Map) of
-        true -> maps:get(Key, Map);
-        false -> Default
-    end.
-
 -spec info(string()) -> ok.
 info(Message) ->
     info(Message, []).

test/elvis_SUITE.erl

@@ -172,6 +172,7 @@ run_webhook(_Config) ->
         FakeFun1 = fun(_, _, _) -> {ok, []} end,
         meck:expect(egithub, pull_req_files, FakeFun1),
         meck:expect(egithub, pull_req_comments, FakeFun1),
+        meck:expect(egithub, issue_comments, FakeFun1),
         FakeFun2 = fun(_, _, _, _) -> {error, error} end,
         meck:expect(egithub, file_content, FakeFun2),
 

test/examples/fail_line_length.erl

@@ -6,7 +6,8 @@
          function_3/0,
          function_4/0,
          function_5/0,
-         function_6/2
+         function_6/2,
+         function_7/0
         ]).
 
 % Single line comment over 80 characters!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!

test/examples/fail_operator_spaces.erl

@@ -1,19 +1,22 @@
 -module(fail_operator_spaces).
 
--export([function1/2,function2/2, function3/2]).
+-export([function1/2,function2/2, function3/2, function4/2]).
 
 %% No space before and after coma,on a comment.
 
 function1(Should,Fail) ->
-  [Should,Fail].
+    [Should,Fail].
 
 function2(Shouldnt, Fail) ->
-  _Unless = [we, consider]++ [operands, as, well],
-  _WithDash = Shouldnt - Fail.
+    _Unless = [we, consider]++ [operands, as, well],
+    _WithDash = Shouldnt - Fail.
 
 function3(Shouldnt, Fail) ->
-  {
-    Shouldnt ++ "Hello,Dont't Fail" ++ Fail,
-    'hello,don\'t fail',
-    <<"hello,don't fail">>
-  }.
+    {
+      Shouldnt ++ "Hello,Dont't Fail" ++ Fail,
+      'hello,don\'t fail',
+      <<"hello,don't fail">>
+    }.
+
+function4(Should, <<_:10/binary, ",", _/binary>>) ->
+    Should = [$,, "where $, represents the character ,"].

test/style_SUITE.erl

@@ -60,17 +60,6 @@ end_per_suite(Config) ->
 %%%%%%%%%%%%%%%
 %%% Rules
 
-verify_line_numbers(_, _, []) ->
-    ok;
-verify_line_numbers(N, Results, [ExpectedNum|More]) ->
-    #{info := [ActualNum|_]} = lists:nth(N, Results),
-    case ActualNum of
-        ExpectedNum ->
-            verify_line_numbers(N + 1, Results, More);
-        _ ->
-            lager:error("Expecting ~p, got ~p~n", [ExpectedNum, ActualNum])
-    end.
-
 -spec verify_line_length_rule(config()) -> any().
 verify_line_length_rule(_Config) ->
     ElvisConfig = elvis_config:default(),
@@ -81,8 +70,8 @@ verify_line_length_rule(_Config) ->
 
     Result = elvis_style:line_length(ElvisConfig, Path, #{limit => 80}),
     8 = length(Result),
-    #{info := Info, message := Msg} = lists:nth(8, Result),
-    <<"Line 31 is too long:     gb_trees:from_orddict(", _/binary>> =
+    #{info := Info, message := Msg} = lists:nth(7, Result),
+    <<"Line 32 is too long:     gb_trees:from_orddict(", _/binary>> =
         list_to_binary(io_lib:format(Msg, Info)),
 
     WholeLineResult = elvis_style:line_length(ElvisConfig, Path,
@@ -93,7 +82,7 @@ verify_line_length_rule(_Config) ->
     AnyResult = elvis_style:line_length(ElvisConfig, Path,
                                         #{limit => 80,
                                           skip_comments => any}),
-    5 = length(AnyResult).
+    6 = length(AnyResult).
 
 -spec verify_no_tabs_rule(config()) -> any().
 verify_no_tabs_rule(_Config) ->