Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correctly handle strings-with-variables as hash keys in arrow_alignment check #621

Merged
merged 3 commits into from Feb 4, 2017

Conversation

rodjek
Copy link
Owner

@rodjek rodjek commented Jan 13, 2017

This PR fixes two bugs that were preventing correct arrow_alignment check results when processing hashes that used strings containing variables as keys. First, the tokeniser's internal state that keeps track of the column number wasn't being updated to take into account the enclosing ${} characters around variables inside double quoted strings. Secondly, when calculating the correct column number for the arrows, we were only looking at the single token before the arrow, which didn't work for double quoted strings as they are represented by a series of tokens.

Fixes #416
Fixes #424

@rnelson0
Copy link
Collaborator

rnelson0 commented Jan 13, 2017

This doesn't quite fix #416:

$ cat 416.pp
#
class example (
  $external_ip_base,
) {

  bar { 'xxxxxxxxxxx':
    inputs => {
      'ny' => {
        "${external_ip_base}.1"       => 'foo',
        "$internal_ip_base.1"         => 'foo',
        'veryveryverylongstring8:443' => 'foo',
      },
    },
  }
}

$ be puppet-lint 416.pp -f
ERROR: example not in autoload module layout on line 2
FIXED: variable not enclosed in {} on line 10
FIXED: indentation of => is not properly aligned (expected in column 39, but found it in column 38) on line 10

$ cat 416.pp
#
class example (
  $external_ip_base,
) {

  bar { 'xxxxxxxxxxx':
    inputs => {
      'ny' => {
        "${external_ip_base}.1"       => 'foo',
        "${internal_ip_base}.1"                           => 'foo',
        'veryveryverylongstring8:443' => 'foo',
      },
    },
  }
}

$ be puppet-lint 416.pp -f
ERROR: example not in autoload module layout on line 2
WARNING: top-scope variable being used without an explicit namespace on line 10
FIXED: indentation of => is not properly aligned (expected in column 39, but found it in column 59) on line 10

$ cat 416.pp
#
class example (
  $external_ip_base,
) {

  bar { 'xxxxxxxxxxx':
    inputs => {
      'ny' => {
        "${external_ip_base}.1"       => 'foo',
        "${internal_ip_base}.1"                           => 'foo',
        'veryveryverylongstring8:443' => 'foo',
      },
    },
  }
}

#424 doesn't crash but gets just as wonky when you mis-align it. It looks to be too far offset on the fix by the length of the variable with curly braces plus 1.

@rnelson0
Copy link
Collaborator

It looks like we need the same trick in the fix portion when dealing with DP_POST:

diff --git a/lib/puppet-lint/plugins/check_whitespace.rb b/lib/puppet-lint/plugins/check_whitespace.rb
index 732e269..2b4905b 100644
--- a/lib/puppet-lint/plugins/check_whitespace.rb
+++ b/lib/puppet-lint/plugins/check_whitespace.rb
@@ -201,7 +201,19 @@ PuppetLint.new_check(:arrow_alignment) do
   end

   def fix(problem)
-    new_ws_len = (problem[:indent_depth] - (problem[:newline_indent] + problem[:token].prev_code_token.to_manifest.length + 1))
+    param_token = problem[:token].prev_code_token
+    if param_token.type == :DQPOST
+      param_length = 0
+      iter_token = param_token
+      while iter_token.type != :DQPRE do
+        param_length += iter_token.to_manifest.length
+        iter_token = iter_token.prev_token
+      end
+      param_length += iter_token.to_manifest.length
+    else
+      param_length = param_token.to_manifest.length
+    end
+    new_ws_len = (problem[:indent_depth] - (problem[:newline_indent] + param_length + 1))
     new_ws = ' ' * new_ws_len
     if problem[:newline]
       index = tokens.index(problem[:token].prev_code_token.prev_token)

This results in correct alignment in the 416 and 424 test cases. We will need a spec test to handle the -fix branch as well.

@rnelson0
Copy link
Collaborator

The build error is related to bundler 1.14.3 and is hurting pretty much everything building against 1.8.7. I feel confident that can be ignored.

@rodjek I think this update should put the nail in the bug's coffin.

@binford2k binford2k merged commit 080a538 into master Feb 4, 2017
@rnelson0 rnelson0 deleted the issue-416 branch February 13, 2017 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants