diff --git a/lib/puppet-lint/lexer/token.rb b/lib/puppet-lint/lexer/token.rb index d45c2443..e7076c39 100644 --- a/lib/puppet-lint/lexer/token.rb +++ b/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 diff --git a/lib/puppet-lint/plugins/check_classes/parameter_order.rb b/lib/puppet-lint/plugins/check_classes/parameter_order.rb index a1deb2b9..3a8e040c 100644 --- a/lib/puppet-lint/plugins/check_classes/parameter_order.rb +++ b/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 diff --git a/spec/fixtures/test/reports/code_climate.json b/spec/fixtures/test/reports/code_climate.json index 5e658271..e2d0d210 100644 --- a/spec/fixtures/test/reports/code_climate.json +++ b/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." } ] diff --git a/spec/unit/puppet-lint/plugins/check_classes/parameter_order_spec.rb b/spec/unit/puppet-lint/plugins/check_classes/parameter_order_spec.rb index 52be7bcd..88083428 100644 --- a/spec/unit/puppet-lint/plugins/check_classes/parameter_order_spec.rb +++ b/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