Merge pull request #4 from racodond/issue_3_false_negatives_with_pupp…

…et_linelength_check Issue #3 - False negatives with puppet:LineLength check
maestrodev · Jul 18, 2015 · 97982ac · 97982ac
2 parents 5990695 + 20a8fae
commit 97982ac
Show file tree

Hide file tree

Showing 5 changed files with 37 additions and 45 deletions.
diff --git a/puppet-checks/src/main/java/com/iadams/sonarqube/puppet/checks/LineLengthCheck.java b/puppet-checks/src/main/java/com/iadams/sonarqube/puppet/checks/LineLengthCheck.java
@@ -24,11 +24,12 @@
  */
 package com.iadams.sonarqube.puppet.checks;
 
-import com.sonar.sslr.api.AstAndTokenVisitor;
+import com.google.common.io.Files;
+import com.iadams.sonarqube.puppet.CharsetAwareVisitor;
 import com.sonar.sslr.api.AstNode;
 import com.sonar.sslr.api.Grammar;
-import com.sonar.sslr.api.Token;
 import org.sonar.api.server.rule.RulesDefinition;
+import org.sonar.api.utils.SonarException;
 import org.sonar.check.Priority;
 import org.sonar.check.Rule;
 import org.sonar.check.RuleProperty;
@@ -37,65 +38,53 @@
 import org.sonar.squidbridge.annotations.SqaleSubCharacteristic;
 import org.sonar.squidbridge.checks.SquidCheck;
 
-/**
- * @author iwarapter
- */
+import javax.annotation.Nullable;
+import java.io.IOException;
+import java.nio.charset.Charset;
+import java.util.List;
+
 @Rule(
-		key = LineLengthCheck.CHECK_KEY,
+		key = "LineLength",
 		priority = Priority.MINOR,
 		name = "Lines should not be too long",
 		tags = Tags.CONVENTION
 )
 @ActivatedByDefault
 @SqaleSubCharacteristic(RulesDefinition.SubCharacteristics.READABILITY)
 @SqaleConstantRemediation("1min")
-public class LineLengthCheck extends SquidCheck<Grammar> implements AstAndTokenVisitor {
+public class LineLengthCheck extends SquidCheck<Grammar> implements CharsetAwareVisitor {
 
-	public static final String CHECK_KEY = "LineLength";
 	private static final int DEFAULT_MAXIMUM_LINE_LENHGTH = 80;
+	private Charset charset;
 
 	@RuleProperty(
 			key = "maximumLineLength",
 			defaultValue = "" + DEFAULT_MAXIMUM_LINE_LENHGTH)
 	public int maximumLineLength = DEFAULT_MAXIMUM_LINE_LENHGTH;
 
-	public int getMaximumLineLength() {
-		return maximumLineLength;
-	}
-
-	private Token previousToken;
-
 	@Override
-	public void visitFile(AstNode astNode) {
-		previousToken = null;
+	public void setCharset(Charset charset) {
+		this.charset = charset;
 	}
 
 	@Override
-	public void leaveFile(AstNode astNode) {
-		previousToken = null;
-	}
-
-	@Override
-	public void visitToken(Token token) {
-		if (!token.isGeneratedCode()) {
-			if (previousToken != null && previousToken.getLine() != token.getLine()) {
-				// Note that AbstractLineLengthCheck doesn't support tokens which span multiple lines - see SONARPLUGINS-2025
-				String[] lines = previousToken.getValue().split("\r?\n|\r", -1);
-				int length = previousToken.getColumn();
-				for (int line = 0; line < lines.length; line++) {
-					length += lines[line].length();
-					if (length > getMaximumLineLength()) {
-						// Note that method from AbstractLineLengthCheck generates other message - see SONARPLUGINS-1809
-						getContext().createLineViolation(this,
-								"The line contains {0,number,integer} characters which is greater than {1,number,integer} authorized.",
-								previousToken.getLine(),
-								length,
-								getMaximumLineLength());
-					}
-					length = 0;
-				}
+	public void visitFile(@Nullable AstNode astNode) {
+		List<String> lines;
+		try {
+			lines = Files.readLines(getContext().getFile(), charset);
+		} catch (IOException e) {
+			throw new SonarException(e);
+		}
+		for (int i = 0; i < lines.size(); i++) {
+			String line = lines.get(i);
+			if (line.length() > maximumLineLength) {
+				getContext().createLineViolation(this,
+						"The line contains {0,number,integer} characters which is greater than {1,number,integer} authorized.",
+						i + 1,
+						line.length(),
+						maximumLineLength);
 			}
-			previousToken = token;
 		}
 	}
+
 }
diff --git a/puppet-checks/src/main/resources/org/sonar/l10n/pp/rules/puppet/LineLength.html b/puppet-checks/src/main/resources/org/sonar/l10n/pp/rules/puppet/LineLength.html
@@ -1,3 +1 @@
-<p>
-For better readability avoid too long lines.
-</p>
+<p>Having to scroll horizontally makes it harder to get a quick overview and understanding of any piece of code.</p>
diff --git a/puppet-checks/src/test/groovy/com/iadams/sonarqube/puppet/checks/LineLengthCheckSpec.groovy b/puppet-checks/src/test/groovy/com/iadams/sonarqube/puppet/checks/LineLengthCheckSpec.groovy
@@ -44,6 +44,7 @@ class LineLengthCheckSpec extends Specification {
 		expect:
 		CheckMessagesVerifier.verify(file.getCheckMessages())
 				.next().atLine(1).withMessage("The line contains 40 characters which is greater than 30 authorized.")
+				.next().atLine(3).withMessage("The line contains 53 characters which is greater than 30 authorized.")
 				.noMore();
 	}
 }
diff --git a/puppet-checks/src/test/resources/checks/lineLength.pp b/puppet-checks/src/test/resources/checks/lineLength.pp
@@ -1,2 +1,3 @@
 notice( "This contains 40 characters." )
-notice('hello')
+notice('hello')
+/* This comments line is longer than 30 characters */
diff --git a/sonar-puppet-plugin/src/main/resources/com/iadams/sonarqube/puppet/pplint/rules.xml b/sonar-puppet-plugin/src/main/resources/com/iadams/sonarqube/puppet/pplint/rules.xml
@@ -60,9 +60,12 @@ file { '/tmp/foo':
     <name><![CDATA[80 Character Line Limit]]></name>
     <configKey>80chars</configKey>
     <description>
-      <![CDATA[Your manifests should not contain any lines longer than 80 characters.]]>
+      <![CDATA[Your manifests should not contain any lines longer than 80 characters.
+      <p>This rule is deprecated, use <a href="coding_rules#rule_key=puppet:LineLength">LineLength</a> instead.</p>
+      ]]>
     </description>
     <priority>MINOR</priority>
+    <status>DEPRECATED</status>
   </rule>
   <rule>
     <key>arrow_alignment</key>