Merge pull request #123 from tiandrey/fix-optional-params

Fix #122

Author: chelnak

lib/puppet-lint/lexer/token.rb

@@ -30,7 +30,7 @@ class Token
     # etc) in the manifest.
     attr_accessor :next_code_token
 
-    # Public: Gets/sets the previous code tokne (skips whitespace,
+    # Public: Gets/sets the previous code token (skips whitespace,
     # comments, etc) in the manifest.
     attr_accessor :prev_code_token
 

lib/puppet-lint/plugins/check_classes/parameter_order.rb

@@ -37,7 +37,7 @@ def check
           column: token.column,
           description: 'Test the manifest tokens for any parameterised classes or defined types that take ' \
                        'parameters and record a warning if there are any optional parameters listed before required parameters.',
-          help_uri: 'https://puppet.com/docs/puppet/latest/style_guide.html#display-order-of-parameters',
+          help_uri: 'https://puppet.com/docs/puppet/latest/style_guide.html#params-display-order',
         )
       end
     end
@@ -48,17 +48,15 @@ def parameter?(token)
     return false unless token.prev_code_token
 
     [
-      :LPAREN, # First parameter, no type specification
-      :COMMA,  # Subsequent parameter, no type specification
-      :TYPE,   # Parameter with simple type specification
-      :RBRACK, # Parameter with complex type specification
+      :LPAREN,   # First parameter, no type specification
+      :COMMA,    # Subsequent parameter, no type specification
+      :TYPE,     # Parameter with simple type specification
+      :RBRACK,   # Parameter with complex type specification
+      :CLASSREF, # Parameter with custom type specification
     ].include?(token.prev_code_token.type)
   end
 
   def required_parameter?(token)
-    data_type = token.prev_token_of(:TYPE, skip_blocks: true)
-    return false if data_type && data_type.value == 'Optional'
-
     return !(token.prev_code_token && token.prev_code_token.type == :EQUALS) if token.next_code_token.nil? || [:COMMA, :RPAREN].include?(token.next_code_token.type)
 
     false

spec/fixtures/test/reports/code_climate.json

@@ -33,6 +33,6 @@
       }
     },
     "fingerprint": "e34cf289e008446b633d1be7cf58120e",
-    "content": "Test the manifest tokens for any parameterised classes or defined types that take parameters and record a warning if there are any optional parameters listed before required parameters. See [this page](https://puppet.com/docs/puppet/latest/style_guide.html#display-order-of-parameters) for more information about the `parameter_order` check."
+    "content": "Test the manifest tokens for any parameterised classes or defined types that take parameters and record a warning if there are any optional parameters listed before required parameters. See [this page](https://puppet.com/docs/puppet/latest/style_guide.html#params-display-order) for more information about the `parameter_order` check."
   }
 ]

spec/unit/puppet-lint/plugins/check_classes/parameter_order_spec.rb

@@ -135,19 +135,6 @@
       it { expect(problems).to have(0).problem }
     end
 
-    context "#{type} parameter with Optional data type" do
-      let(:code) do
-        <<-END
-          #{type} test(
-            String $test = 'value',
-            Optional[String] $optional,
-          ) { }
-        END
-      end
-
-      it { expect(problems).to have(0).problems }
-    end
-
     context "#{type} parameter with array operation" do
       let(:code) do
         <<-END
@@ -165,5 +152,72 @@
 
       it { expect(problems).to have(0).problems }
     end
+
+    context "#{type} parameters of Optional type are just regular parameters" do
+      let(:code) do
+        <<-END
+          #{type} test (
+            String $param1 = '',
+            Optional[Boolean] $param2,
+          ) { }
+        END
+      end
+
+      it { expect(problems).to have(1).problems }
+    end
+
+    [['Custom::Type1', 'Custom::Type2'], ['Custom::Type1', 'String'], ['String', 'Custom::Type2']].each_with_index do |testtypes, i| # rubocop:disable Performance/CollectionLiteralInLoop
+      context "#{type} parameters of custom type - no values - #{i}" do
+        let(:code) do
+          <<-END
+            #{type} test (
+              #{testtypes[0]} $param1,
+              #{testtypes[0]} $param2,
+            ) { }
+          END
+        end
+
+        it { expect(problems).to have(0).problems }
+      end
+
+      context "#{type} parameters of custom type - two values - #{i}" do
+        let(:code) do
+          <<-END
+            #{type} test (
+              #{testtypes[0]} $param1 = 'foo',
+              #{testtypes[1]} $param2 = 'bar',
+            ) { }
+          END
+        end
+
+        it { expect(problems).to have(0).problems }
+      end
+
+      context "#{type} parameters of custom type - one value, wrong order - #{i}" do
+        let(:code) do
+          <<-END
+            #{type} test (
+              #{testtypes[0]} $param1 = 'foo',
+              #{testtypes[1]} $param2,
+            ) { }
+          END
+        end
+
+        it { expect(problems).to have(1).problems }
+      end
+
+      context "#{type} parameters of custom type - one value, right order - #{i}" do
+        let(:code) do
+          <<-END
+            #{type} test (
+              #{testtypes[0]} $param1,
+              #{testtypes[1]} $param2 = 'bar',
+            ) { }
+          END
+        end
+
+        it { expect(problems).to have(0).problems }
+      end
+    end
   end
 end