fix bugs and suggestions

Author: bormilan

src/elvis_style.erl

@@ -2769,15 +2769,14 @@ macro_definition_parentheses(Rule, ElvisConfig) ->
 -spec strict_module_layout(elvis_rule:t(), elvis_config:t()) -> [elvis_result:item()].
 strict_module_layout(Rule, ElvisConfig) ->
     Nodes = ktn_code:content(elvis_code:root(Rule, ElvisConfig)),
+    Order = elvis_rule:option(order, Rule),
 
     NotInPlace =
         fun(Node, {Prev, Result}) ->
-            Order = elvis_rule:option(order, Rule),
-
             Type = ktn_code:type(Node),
             Category = get_category(Type),
 
-            case not ignore(Type, Category, Order) of
+            case not ignore_category(Type, Category, Order) of
                 true -> valid_order(Category, Prev, Node, Result, Order);
                 false -> {Prev, Result}
             end
@@ -2786,7 +2785,7 @@ strict_module_layout(Rule, ElvisConfig) ->
     {_, ResultNodes} = lists:foldl(NotInPlace, {nil, []}, Nodes),
 
     lists:map(
-        fun({CorrectPrev, ActualPrev, NodeTypeCategory, Node}) ->
+        fun({ActualPrev, NodeTypeCategory, Node}) ->
             elvis_result:new_item(
                 "A ~p type element was found after a ~p type element."
                 " That doesn't respect the expected module layout: ~p",
@@ -2808,11 +2807,11 @@ valid_order(Category, Prev, Node, Result, Order) ->
             Category =:= Prev
     of
         true -> {Category, Result};
-        false -> {Category, [{CorrectPrev, Prev, Category, Node} | Result]}
+        false -> {Category, [{Prev, Category, Node} | Result]}
     end.
 
-ignore(Type, Category, Order) ->
-    lists:member(Type, [comment, ifdef, ifndef, 'else', 'if', elif, endif]) orelse
+ignore_category(Type, Category, Order) ->
+    lists:member(Type, [comment, ifdef, ifndef, else_attr, 'if', elif, endif]) orelse
         not lists:member(Category, Order).
 
 get_category(Type) ->
@@ -2827,9 +2826,16 @@ get_category(Type) ->
         {function, body},
         {spec, body},
         {export, exports},
-        {export_type, exports},
+        {export_type, export_types},
         {define, defines},
-        {comment, comment}
+        {comment, comment},
+        {doc, comment},
+        {moduledoc, moduledoc},
+        {import, import},
+        {compile, compile},
+        {vsn, vsn},
+        {behaviour, behaviour},
+        {record, record}
     ],
     proplists:get_value(Type, Categories, custom).
 

test/examples/fail_strict_module_layout.erl

@@ -5,13 +5,27 @@
 -export_type([my_type/0]).
 
 -export([some_fun/0]).
+-import(test_module, []).
+
+-behaviour(gen_event).
+-export([init/1, handle_event/2]).
 
 -type my_type() :: ok.
+-export([handle_call/2]).
 
 -mixin([]).
+-doc("doc").
+-vsn(1.0).
+-moduledoc("").
 
 -spec some_fun() -> my_type().
 some_fun() -> ok.
 
 -dialyzer({nowarn_function, []}).
 
+-compile("").
+
+handle_call(_, _) -> ok.
+handle_event(_, _) -> ok.
+init(_) -> ok.
+

test/examples/fail_strict_module_layout_allways.erl

@@ -0,0 +1,6 @@
+-module(fail_strict_module_layout_allways).
+
+-type bananas() :: bananas.
+-export_type([bananas/0, tomatoes/0]).
+-type tomatoes() :: tomatoes.
+

test/examples/pass_strict_module_layout.erl

@@ -1,4 +1,7 @@
 -module(pass_strict_module_layout).
+-moduledoc("").
+
+-vsn(1.0).
 
 -include("./prefer_include.hrl").
 
@@ -8,6 +11,8 @@
 -hank([]).
 -import(test_module, []).
 
+-behaviour(gen_event).
+
 -type my_type() :: ok.
 -opaque my_opaque() :: definition.
 
@@ -16,11 +21,26 @@
 -export_type([name/0]).
 -endif.
 
+-if(ok).
+-type apple() :: apple.
+-export_type([apple/0]).
+-elif(notok).
+-type orange() :: orange.
+-export_type([orange/0]).
+-else.
+-type banana() :: banana.
+-export_type([banana/0]).
+-endif.
+
 -export_type([my_opaque/0]).
 -export([some_fun/0, some_fun_2/0, some_fun_3/0, some_fun_4/0]).
+-export([init/1, handle_call/2, handle_event/2]).
+
 
 -define(MACRO, ok).
 
+-doc("doc").
+
 -spec some_fun() -> my_type().
 some_fun() -> ?MACRO.
 
@@ -32,3 +52,7 @@ some_fun_3() -> ok.
 
 -spec some_fun_4() -> my_type().
 some_fun_4() -> ok.
+
+handle_call(_, _) -> ok.
+handle_event(_, _) -> ok.
+init(_) -> ok.

test/style_SUITE.erl

@@ -3223,14 +3223,62 @@ verify_strict_module_layout(Config) ->
             Config, elvis_style, strict_module_layout, #{order => Order}, PassPath
         ),
 
+    % causing trouble with a poor order
+    [
+        #{line_num := 2},
+        #{line_num := 4},
+        #{line_num := 14},
+        #{line_num := 21},
+        #{line_num := 42},
+        #{line_num := 44}
+    ] =
+        elvis_test_utils:elvis_core_apply_rule(
+            Config,
+            elvis_style,
+            strict_module_layout,
+            #{order => [module, vsn, moduledoc, behaviour, comment, export_types, exports, body]},
+            PassPath
+        ),
+
+    % empty order list, same as disabling thiss rule
+    [] =
+        elvis_test_utils:elvis_core_apply_rule(
+            Config, elvis_style, strict_module_layout, #{order => []}, PassPath
+        ),
+
     FailModule = fail_strict_module_layout,
     FailPath = atom_to_list(FailModule) ++ "." ++ Ext,
 
-    [_, _, _, _, _, _] =
+    [_, _, _, _, _, _, _] =
         elvis_test_utils:elvis_core_apply_rule(
             Config, elvis_style, strict_module_layout, #{order => Order}, FailPath
         ),
 
+    % check a lot of stuff
+    [_, _, _, _, _, _, _, _, _, _, _, _] =
+        elvis_test_utils:elvis_core_apply_rule(
+            Config,
+            elvis_style,
+            strict_module_layout,
+            #{
+                order => [
+                    module,
+                    moduledoc,
+                    vsn,
+                    compile,
+                    behaviour,
+                    record,
+                    import,
+                    types,
+                    comment,
+                    exports,
+                    body
+                ]
+            },
+            FailPath
+        ),
+
+    % let's check just a few parts
     [] =
         elvis_test_utils:elvis_core_apply_rule(
             Config, elvis_style, strict_module_layout, #{order => [module]}, FailPath
@@ -3243,6 +3291,58 @@ verify_strict_module_layout(Config) ->
             strict_module_layout,
             #{order => [module, exports, body]},
             FailPath
+        ),
+
+    % empty order list, same as disabling thiss rule
+    [] =
+        elvis_test_utils:elvis_core_apply_rule(
+            Config, elvis_style, strict_module_layout, #{order => []}, FailPath
+        ),
+
+    % this allways will fail
+    AllwaysFailModule = fail_strict_module_layout_allways,
+    AllwaysFailPath = atom_to_list(AllwaysFailModule) ++ "." ++ Ext,
+
+    [#{line_num := 5}] =
+        elvis_test_utils:elvis_core_apply_rule(
+            Config,
+            elvis_style,
+            strict_module_layout,
+            #{order => [module, types, export_types]},
+            AllwaysFailPath
+        ),
+
+    [#{line_num := 3}, #{line_num := 4}] =
+        elvis_test_utils:elvis_core_apply_rule(
+            Config,
+            elvis_style,
+            strict_module_layout,
+            #{order => [module, export_types, types]},
+            AllwaysFailPath
+        ),
+
+    % if we don't add the module part, this will not fail
+    % because 'type' will not have any defined previous elements,
+    % so it basically not violate the rule
+    [] =
+        elvis_test_utils:elvis_core_apply_rule(
+            Config,
+            elvis_style,
+            strict_module_layout,
+            #{order => [types, export_types]},
+            AllwaysFailPath
+        ),
+
+    % repeating a keyword does not have any effect,
+    % because we get the first entry from the order list when checking
+    % if the the previous element matches with the right order or not
+    [#{line_num := 5}] =
+        elvis_test_utils:elvis_core_apply_rule(
+            Config,
+            elvis_style,
+            strict_module_layout,
+            #{order => [module, types, export_types, types]},
+            AllwaysFailPath
         ).
 
 verify_elvis_attr_atom_naming_convention(Config) ->