From 7974217326513c78379ecee000741bf1abf2a4b0 Mon Sep 17 00:00:00 2001
From: Matthias Baur <m.baur@syseleven.de>
Date: Mon, 14 Jan 2019 15:56:05 +0100
Subject: [PATCH] Add option to reverse the check

Meaning: Warn if there are :: in front of a class definition.

Fixes #17.
---
 README.md                                     |  10 +-
 .../plugins/check_absolute_classname.rb       |  38 +-
 .../relative_classname_inclusion_spec.rb      | 500 +++++++++++++-----
 3 files changed, 398 insertions(+), 150 deletions(-)

diff --git a/README.md b/README.md
index 55130b8..d81eb4f 100644
--- a/README.md
+++ b/README.md
@@ -28,7 +28,7 @@ gem 'puppet-lint-absolute_classname-check', :require => false
 
 ### Relative class name inclusion
 
-Including a class by a relative name might lead to unexpected results [in Puppet 3](https://docs.puppet.com/puppet/3/lang_namespaces.html#relative-name-lookup-and-incorrect-name-resolution). This plugin is **not** recommended for use with Puppet code that has dropped support for Puppet 3 (EOL 20161231).
+Including a class by a relative name might lead to unexpected results [in Puppet 3](https://docs.puppet.com/puppet/3/lang_namespaces.html#relative-name-lookup-and-incorrect-name-resolution).
 
 #### What you have done
 
@@ -42,6 +42,14 @@ include foobar
 include ::foobar
 ```
 
+#### Reverse this check
+
+This check can be reversed to check for Puppet > 4.
+
+```ruby
+PuppetLint.configuration.absolute_classname_reverse = true
+```
+
 #### Disabling the check
 
 To disable this check, you can add `--no-relative_classname_inclusion-check` to your puppet-lint command line.
diff --git a/lib/puppet-lint/plugins/check_absolute_classname.rb b/lib/puppet-lint/plugins/check_absolute_classname.rb
index b100ebe..c546dea 100644
--- a/lib/puppet-lint/plugins/check_absolute_classname.rb
+++ b/lib/puppet-lint/plugins/check_absolute_classname.rb
@@ -1,10 +1,19 @@
 PuppetLint.new_check(:relative_classname_inclusion) do
   def check
     tokens.each_with_index do |token, token_idx|
+      reverse = PuppetLint.configuration.absolute_classname_reverse || false
+      if reverse
+        pattern = '^(?!::)'
+        message = 'class included by absolute name (::$class)'
+      else
+        pattern = '^::'
+        message = 'class included by relative name'
+      end
       if [:NAME,:FUNCTION_NAME].include?(token.type) && ['include','contain','require'].include?(token.value)
         s = token.next_code_token
         next if s.nil?
         next if s.type == :FARROW
+
         in_function = 0
         while s.type != :NEWLINE
           n = s.next_code_token
@@ -13,12 +22,12 @@ def check
               in_function += 1
             elsif in_function > 0 && n && n.type == :RPAREN
               in_function -= 1
-            elsif in_function == 0 && s.value !~ /^::/
+            elsif in_function.zero? && s.value !~ /#{pattern}/
               notify :warning, {
-                :message => 'class included by relative name',
-                :line    => s.line,
-                :column  => s.column,
-                :token   => s,
+                message: message,
+                line: s.line,
+                column: s.column,
+                token: s
               }
             end
           end
@@ -27,21 +36,26 @@ def check
       elsif token.type == :CLASS and token.next_code_token.type == :LBRACE
         s = token.next_code_token
         while s.type != :COLON
-          if (s.type == :NAME || s.type == :SSTRING) && s.value !~ /^::/
+          if (s.type == :NAME || s.type == :SSTRING) && s.value !~ /#{pattern}/
             notify :warning, {
-              :message => 'class included by relative name',
-              :line    => s.line,
-              :column  => s.column,
-              :token   => s,
+              message: message,
+              line: s.line,
+              column: s.column,
+              token: s
             }
           end
           s = s.next_token
         end
       end
     end
-  end 
+  end
 
   def fix(problem)
-    problem[:token].value = '::'+problem[:token].value
+    reverse = PuppetLint.configuration.absolute_classname_reverse || false
+    problem[:token].value = if reverse
+                              problem[:token].value[2..-1]
+                            else
+                              '::' + problem[:token].value
+                            end
   end
 end
diff --git a/spec/puppet-lint/plugins/check_absolute_classname/relative_classname_inclusion_spec.rb b/spec/puppet-lint/plugins/check_absolute_classname/relative_classname_inclusion_spec.rb
index de410a8..90bbd6d 100644
--- a/spec/puppet-lint/plugins/check_absolute_classname/relative_classname_inclusion_spec.rb
+++ b/spec/puppet-lint/plugins/check_absolute_classname/relative_classname_inclusion_spec.rb
@@ -1,199 +1,425 @@
 require 'spec_helper'
 
 describe 'relative_classname_inclusion' do
-  let(:msg) { 'class included by relative name' }
+  describe '(default)' do
+    let(:msg) { 'class included by relative name' }
 
-  context 'with fix disabled' do
-    context 'when absolute names are used' do
-      let(:code) do
-        <<-EOS
-        include ::foobar
-        include('::foobar')
-        include(foobar(baz))
-        include(foobar('baz'))
+    context 'with fix disabled' do
+      context 'when absolute names are used' do
+        let(:code) do
+          <<-EOS
+          include ::foobar
+          include('::foobar')
+          include(foobar(baz))
+          include(foobar('baz'))
 
-        include ::foo, ::bar
-        include('::foo', '::bar')
+          include ::foo, ::bar
+          include('::foo', '::bar')
 
-        class { '::foobar': }
+          class { '::foobar': }
 
-        class foobar {
-        }
+          class foobar {
+          }
 
-        contain ::foobar
-        contain('::foobar')
-        contain(foobar(baz))
-        contain(foobar('baz'))
+          contain ::foobar
+          contain('::foobar')
+          contain(foobar(baz))
+          contain(foobar('baz'))
 
-        require ::foobar
-        require('::foobar')
-        require(foobar(baz))
-        require(foobar('baz'))
-        EOS
-      end
+          require ::foobar
+          require('::foobar')
+          require(foobar(baz))
+          require(foobar('baz'))
+          EOS
+        end
 
-      it 'should not detect any problems' do
-        expect(problems).to have(0).problems
+        it 'should not detect any problems' do
+          expect(problems).to have(0).problems
+        end
       end
-    end
 
-    context 'when relative names are used' do
-      let(:code) do
-        <<-EOS
-        include foobar
-        include(foobar)
-        class { 'foobar': }
-        contain foobar
-        contain(foobar)
-        require foobar
-        require(foobar)
-        EOS
+      context 'when relative names are used' do
+        let(:code) do
+          <<-EOS
+          include foobar
+          include(foobar)
+          class { 'foobar': }
+          contain foobar
+          contain(foobar)
+          require foobar
+          require(foobar)
+          EOS
+        end
+
+        it 'should detect 7 problems' do
+          expect(problems).to have(7).problems
+        end
+
+        it 'should create warnings' do
+          expect(problems).to contain_warning(msg).on_line(1).in_column(19)
+          expect(problems).to contain_warning(msg).on_line(2).in_column(19)
+          expect(problems).to contain_warning(msg).on_line(3).in_column(19)
+          expect(problems).to contain_warning(msg).on_line(4).in_column(19)
+          expect(problems).to contain_warning(msg).on_line(5).in_column(19)
+          expect(problems).to contain_warning(msg).on_line(6).in_column(19)
+          expect(problems).to contain_warning(msg).on_line(7).in_column(19)
+        end
       end
 
-      it 'should detect 7 problems' do
-        expect(problems).to have(7).problems
+      context 'when the require metadata parameter is used' do
+        let(:code) do
+          <<-EOS
+          file { '/path':
+            ensure  => present,
+            require => Shellvar['http_proxy'],
+          }
+          EOS
+        end
+
+        it 'should detect no problems' do
+          expect(problems).to have(0).problems
+        end
       end
 
-      it 'should create warnings' do
-        expect(problems).to contain_warning(msg).on_line(1).in_column(17)
-        expect(problems).to contain_warning(msg).on_line(2).in_column(17)
-        expect(problems).to contain_warning(msg).on_line(3).in_column(17)
-        expect(problems).to contain_warning(msg).on_line(4).in_column(17)
-        expect(problems).to contain_warning(msg).on_line(5).in_column(17)
-        expect(problems).to contain_warning(msg).on_line(6).in_column(17)
-        expect(problems).to contain_warning(msg).on_line(7).in_column(17)
+      context 'when require is a hash key' do
+        let(:code) do
+          <<-EOS
+          $defaults = {
+            require => Exec['apt_update'],
+          }
+          $defaults = {
+            'require' => Exec['apt_update'],
+          }
+          EOS
+        end
+
+        it 'should detect no problems' do
+          expect(problems).to have(0).problems
+        end
       end
     end
 
-    context 'when the require metadata parameter is used' do
-      let(:code) do
-        <<-EOS
-        file { '/path':
-          ensure  => present,
-          require => Shellvar['http_proxy'],
-        }
-        EOS
+    context 'with fix enabled' do
+      before do
+        PuppetLint.configuration.fix = true
       end
 
-      it 'should detect no problems' do
-        expect(problems).to have(0).problems
+      after do
+        PuppetLint.configuration.fix = false
+      end
+
+      context 'when absolute names are used' do
+        let(:code) do
+          <<-EOS
+          include ::foobar
+          include('::foobar')
+          include(foobar(baz))
+          include(foobar('baz'))
+
+          include ::foo, ::bar
+          include('::foo', '::bar')
+
+          class { '::foobar': }
+
+          class foobar {
+          }
+
+          contain ::foobar
+          contain('::foobar')
+          contain(foobar(baz))
+          contain(foobar('baz'))
+
+          require ::foobar
+          require('::foobar')
+          require(foobar(baz))
+          require(foobar('baz'))
+          EOS
+        end
+
+        it 'should not detect any problems' do
+          expect(problems).to have(0).problems
+        end
+      end
+
+      context 'when relative names are used' do
+        let(:code) do
+          <<-EOS
+          include foobar
+          include(foobar)
+          class { 'foobar': }
+          contain foobar
+          contain(foobar)
+          require foobar
+          require(foobar)
+          EOS
+        end
+
+        it 'should detect 7 problems' do
+          expect(problems).to have(7).problems
+        end
+
+        it 'should fix the problems' do
+          expect(problems).to contain_fixed(msg).on_line(1).in_column(19)
+          expect(problems).to contain_fixed(msg).on_line(2).in_column(19)
+          expect(problems).to contain_fixed(msg).on_line(3).in_column(19)
+          expect(problems).to contain_fixed(msg).on_line(4).in_column(19)
+          expect(problems).to contain_fixed(msg).on_line(5).in_column(19)
+          expect(problems).to contain_fixed(msg).on_line(6).in_column(19)
+          expect(problems).to contain_fixed(msg).on_line(7).in_column(19)
+        end
+
+        it 'should should add colons' do
+          expect(manifest).to eq(
+          <<-EOS
+          include ::foobar
+          include(::foobar)
+          class { '::foobar': }
+          contain ::foobar
+          contain(::foobar)
+          require ::foobar
+          require(::foobar)
+          EOS
+          )
+        end
       end
     end
 
-    context 'when require is a hash key' do
+    describe '(#12) behavior of lookup("foo", {merge => unique}).include' do
+      let(:msg) { '(#12) class included with lookup("foo", {merge => unique}).include' }
+
       let(:code) do
         <<-EOS
-        $defaults = {
-          require => Exec['apt_update'],
-        }
-        $defaults = {
-          'require' => Exec['apt_update'],
-        }
+        lookup(foo, {merge => unique}).include
         EOS
       end
 
-      it 'should detect no problems' do
+      it 'should not detect any problems' do
         expect(problems).to have(0).problems
       end
     end
   end
 
-  context 'with fix enabled' do
+  describe '(reversed)' do
     before do
-      PuppetLint.configuration.fix = true
+      PuppetLint.configuration.absolute_classname_reverse = true
     end
+    let(:msg) { 'class included by absolute name (::$class)' }
 
-    after do
-      PuppetLint.configuration.fix = false
-    end
+    context 'with fix disabled' do
+      context 'when absolute names are used' do
+        let(:code) do
+          <<-EOS
+          include ::foobar
+          include('::foobar')
+          include(foobar(baz))
+          include(foobar('baz'))
 
-    context 'when absolute names are used' do
-      let(:code) do
-        <<-EOS
-        include ::foobar
-        include('::foobar')
-        include(foobar(baz))
-        include(foobar('baz'))
+          include ::foo, ::bar
+          include('::foo', '::bar')
 
-        include ::foo, ::bar
-        include('::foo', '::bar')
+          class { '::foobar': }
 
-        class { '::foobar': }
+          class foobar {
+          }
 
-        class foobar {
-        }
+          contain ::foobar
+          contain('::foobar')
+          contain(foobar(baz))
+          contain(foobar('baz'))
 
-        contain ::foobar
-        contain('::foobar')
-        contain(foobar(baz))
-        contain(foobar('baz'))
+          require ::foobar
+          require('::foobar')
+          require(foobar(baz))
+          require(foobar('baz'))
+          EOS
+        end
 
-        require ::foobar
-        require('::foobar')
-        require(foobar(baz))
-        require(foobar('baz'))
-        EOS
+        it 'should detect 11 problems' do
+          expect(problems).to have(11).problems
+        end
+
+        it 'should create warnings' do
+          expect(problems).to contain_warning(msg).on_line(1).in_column(19)
+          expect(problems).to contain_warning(msg).on_line(2).in_column(19)
+          expect(problems).to contain_warning(msg).on_line(6).in_column(19)
+          expect(problems).to contain_warning(msg).on_line(6).in_column(26)
+          expect(problems).to contain_warning(msg).on_line(7).in_column(19)
+          expect(problems).to contain_warning(msg).on_line(7).in_column(28)
+          expect(problems).to contain_warning(msg).on_line(9).in_column(19)
+          expect(problems).to contain_warning(msg).on_line(14).in_column(19)
+          expect(problems).to contain_warning(msg).on_line(15).in_column(19)
+          expect(problems).to contain_warning(msg).on_line(19).in_column(19)
+          expect(problems).to contain_warning(msg).on_line(20).in_column(19)
+        end
       end
 
-      it 'should not detect any problems' do
-        expect(problems).to have(0).problems
+      context 'when relative names are used' do
+        let(:code) do
+          <<-EOS
+          include foobar
+          include(foobar)
+          class { 'foobar': }
+          contain foobar
+          contain(foobar)
+          require foobar
+          require(foobar)
+          EOS
+        end
+
+        it 'should not detect a problem' do
+          expect(problems).to have(0).problems
+        end
+      end
+
+      context 'when the require metadata parameter is used' do
+        let(:code) do
+          <<-EOS
+          file { '/path':
+            ensure  => present,
+            require => Shellvar['http_proxy'],
+          }
+          EOS
+        end
+
+        it 'should detect no problems' do
+          expect(problems).to have(0).problems
+        end
+      end
+
+      context 'when require is a hash key' do
+        let(:code) do
+          <<-EOS
+          $defaults = {
+            require => Exec['apt_update'],
+          }
+          $defaults = {
+            'require' => Exec['apt_update'],
+          }
+          EOS
+        end
+
+        it 'should detect no problems' do
+          expect(problems).to have(0).problems
+        end
       end
     end
 
-    context 'when relative names are used' do
-      let(:code) do
-        <<-EOS
-        include foobar
-        include(foobar)
-        class { 'foobar': }
-        contain foobar
-        contain(foobar)
-        require foobar
-        require(foobar)
-        EOS
+    context 'with fix enabled' do
+      before do
+        PuppetLint.configuration.fix = true
       end
 
-      it 'should detect 7 problems' do
-        expect(problems).to have(7).problems
+      after do
+        PuppetLint.configuration.fix = false
       end
 
-      it 'should fix the problems' do
-        expect(problems).to contain_fixed(msg).on_line(1).in_column(17)
-        expect(problems).to contain_fixed(msg).on_line(2).in_column(17)
-        expect(problems).to contain_fixed(msg).on_line(3).in_column(17)
-        expect(problems).to contain_fixed(msg).on_line(4).in_column(17)
-        expect(problems).to contain_fixed(msg).on_line(5).in_column(17)
-        expect(problems).to contain_fixed(msg).on_line(6).in_column(17)
-        expect(problems).to contain_fixed(msg).on_line(7).in_column(17)
+      context 'when absolute names are used' do
+        let(:code) do
+          <<-EOS
+          include ::foobar
+          include('::foobar')
+          include(foobar(baz))
+          include(foobar('baz'))
+
+          include ::foo, ::bar
+          include('::foo', '::bar')
+
+          class { '::foobar': }
+
+          class foobar {
+          }
+
+          contain ::foobar
+          contain('::foobar')
+          contain(foobar(baz))
+          contain(foobar('baz'))
+
+          require ::foobar
+          require('::foobar')
+          require(foobar(baz))
+          require(foobar('baz'))
+          EOS
+        end
+
+        it 'should detect 11 problems' do
+          expect(problems).to have(11).problems
+        end
+
+        it 'should fix the problems' do
+          expect(problems).to contain_fixed(msg).on_line(1).in_column(19)
+          expect(problems).to contain_fixed(msg).on_line(2).in_column(19)
+          expect(problems).to contain_fixed(msg).on_line(6).in_column(19)
+          expect(problems).to contain_fixed(msg).on_line(6).in_column(26)
+          expect(problems).to contain_fixed(msg).on_line(7).in_column(19)
+          expect(problems).to contain_fixed(msg).on_line(7).in_column(28)
+          expect(problems).to contain_fixed(msg).on_line(9).in_column(19)
+          expect(problems).to contain_fixed(msg).on_line(14).in_column(19)
+          expect(problems).to contain_fixed(msg).on_line(15).in_column(19)
+          expect(problems).to contain_fixed(msg).on_line(19).in_column(19)
+          expect(problems).to contain_fixed(msg).on_line(20).in_column(19)
+        end
+
+        it 'should should remove colons' do
+          expect(manifest).to eq(
+          <<-EOS
+          include foobar
+          include('foobar')
+          include(foobar(baz))
+          include(foobar('baz'))
+
+          include foo, bar
+          include('foo', 'bar')
+
+          class { 'foobar': }
+
+          class foobar {
+          }
+
+          contain foobar
+          contain('foobar')
+          contain(foobar(baz))
+          contain(foobar('baz'))
+
+          require foobar
+          require('foobar')
+          require(foobar(baz))
+          require(foobar('baz'))
+          EOS
+          )
+        end
       end
 
-      it 'should should add colons' do
-        expect(manifest).to eq(
-        <<-EOS
-        include ::foobar
-        include(::foobar)
-        class { '::foobar': }
-        contain ::foobar
-        contain(::foobar)
-        require ::foobar
-        require(::foobar)
-        EOS
-        )
+      context 'when relative names are used' do
+        let(:code) do
+          <<-EOS
+          include foobar
+          include(foobar)
+          class { 'foobar': }
+          contain foobar
+          contain(foobar)
+          require foobar
+          require(foobar)
+          EOS
+        end
+
+        it 'should not detect any problems' do
+          expect(problems).to have(0).problems
+        end
       end
     end
-  end
 
-  describe '(#12) behavior of lookup("foo", {merge => unique}).include' do
-    let(:msg) { '(#12) class included with lookup("foo", {merge => unique}).include' }
+    describe '(#12) behavior of lookup("foo", {merge => unique}).include' do
+      let(:msg) { '(#12) class included with lookup("foo", {merge => unique}).include' }
 
-    let(:code) do
-      <<-EOS
-      lookup(foo, {merge => unique}).include
-      EOS
-    end
+      let(:code) do
+        <<-EOS
+        lookup(foo, {merge => unique}).include
+        EOS
+      end
 
-    it 'should not detect any problems' do
-      expect(problems).to have(0).problems
+      it 'should not detect any problems' do
+        expect(problems).to have(0).problems
+      end
     end
   end
 end