Merge pull request #303 from RealyUniqueName/require-no-spaces

Spacing checks: require no spaces

Author: adireddy

.gitignore

@@ -18,3 +18,5 @@ target/
 check-style-report.json
 check-style-report.xml
 coverage.json
+
+display.hxml

checkstyle.json

@@ -517,6 +517,9 @@
 			"checkstyle.Main",
 			"checkstyle.Checker",
 			"checkstyle.ChecksInfo",
+			"checkstyle.checks.Check",
+			"checkstyle.checks.Directive",
+			"checkstyle.checks.whitespace.SpacingCheck",
 			"checkstyle.checks.imports.UnusedImportCheck",
 			"TestMain",
 			"misc.ExtensionsTest",

resources/default-config.json

@@ -442,13 +442,13 @@
 		},
 		{
 			"props": {
-				"spaceIfCondition": true,
+				"spaceIfCondition": 0,
 				"spaceAroundBinop": true,
-				"spaceForLoop": true,
+				"spaceForLoop": 0,
 				"ignoreRangeOperator": true,
-				"spaceWhileLoop": true,
-				"spaceCatch": true,
-				"spaceSwitchCase": true,
+				"spaceWhileLoop": 0,
+				"spaceCatch": 0,
+				"spaceSwitchCase": 0,
 				"noSpaceAroundUnop": true
 			},
 			"type": "Spacing"

src/checkstyle/Main.hx

@@ -11,6 +11,7 @@ import checkstyle.reporter.TextReporter;
 import checkstyle.reporter.XMLReporter;
 import checkstyle.reporter.CodeClimateReporter;
 import checkstyle.reporter.ExitCodeReporter;
+import checkstyle.errors.Error;
 import haxe.CallStack;
 import haxe.Json;
 import hxargs.Args;
@@ -181,7 +182,14 @@ class Main {
 			if (!checkFields.contains(prop)) {
 				failWith('Check ${check.getModuleName()} has no property named \'$prop\'');
 			}
-			Reflect.setField(check, prop, val);
+			try {
+				check.configureProperty(prop, val);
+			}
+			catch (e:Dynamic) {
+				var message = 'Failed to configure $prop setting for ${check.getModuleName()}: ';
+				message += (Std.is(e, Error) ? (e:Error).message : Std.string(e));
+				failWith(message);
+			}
 		}
 		if (defaultSeverity != null && !props.contains("severity")) check.severity = defaultSeverity;
 	}

src/checkstyle/checks/Check.hx

@@ -31,6 +31,10 @@ class Check {
 		desc = haxe.rtti.Meta.getType(Type.getClass(this)).desc[0];
 	}
 
+	public function configureProperty(name:String, value:Dynamic) {
+		Reflect.setField(this, name, value);
+	}
+
 	public function run(checker:Checker):Array<CheckMessage> {
 		this.checker = checker;
 		messages = [];

src/checkstyle/checks/Directive.hx

@@ -0,0 +1,33 @@
+package checkstyle.checks;
+
+import Type.ValueType;
+import checkstyle.errors.InvalidDirectiveError;
+
+enum Directive {
+	SHOULD;
+	SHOULD_NOT;
+	ANY;
+}
+
+class DirectiveTools {
+	static var MAPPING:Map<String, Directive> = [
+		"should" => SHOULD,
+		"shouldNot" => SHOULD_NOT,
+		"any" => ANY
+	];
+
+	public static function fromDynamic(value:Dynamic):Directive {
+		return switch (Type.typeof(value)) {
+			case ValueType.TClass(String): getValidated(value);
+			//support for legacy configs when such settings were boolean
+			case ValueType.TBool: (value ? SHOULD : ANY);
+			case _: ANY;
+		}
+	}
+
+	static function getValidated(value:String):Directive {
+		var directive = MAPPING.get(value);
+		if (directive != null) return directive;
+		throw new InvalidDirectiveError('Invalid directive: $value');
+	}
+}

src/checkstyle/checks/whitespace/SpacingCheck.hx

@@ -1,37 +1,50 @@
 package checkstyle.checks.whitespace;
 
+import Type.ValueType;
 import checkstyle.utils.ExprUtils;
 import haxe.macro.Expr;
 import haxe.macro.Printer;
 import haxe.macro.Expr.Binop;
 import haxe.macro.Expr.Unop;
+import checkstyle.checks.Directive;
 
 @name("Spacing")
 @desc("Spacing check on if, for, while, switch, try statements and around operators.")
 class SpacingCheck extends Check {
 
 	public var spaceAroundBinop:Bool;
 	public var noSpaceAroundUnop:Bool;
-	public var spaceIfCondition:Bool;
-	public var spaceForLoop:Bool;
-	public var spaceWhileLoop:Bool;
-	public var spaceSwitchCase:Bool;
-	public var spaceCatch:Bool;
+	public var spaceIfCondition:Directive;
+	public var spaceForLoop:Directive;
+	public var spaceWhileLoop:Directive;
+	public var spaceSwitchCase:Directive;
+	public var spaceCatch:Directive;
 	public var ignoreRangeOperator:Bool;
 
 	public function new() {
 		super(AST);
 		spaceAroundBinop = true;
 		noSpaceAroundUnop = true;
-		spaceIfCondition = true;
-		spaceForLoop = true;
-		spaceWhileLoop = true;
-		spaceSwitchCase = true;
-		spaceCatch = true;
+		spaceIfCondition = SHOULD;
+		spaceForLoop = SHOULD;
+		spaceWhileLoop = SHOULD;
+		spaceSwitchCase = SHOULD;
+		spaceCatch = SHOULD;
 		ignoreRangeOperator = true;
 		categories = [Category.STYLE, Category.CLARITY];
 	}
 
+	override public function configureProperty(name:String, value:Dynamic) {
+		var currentValue = Reflect.field(this, name);
+
+		switch (Type.typeof(currentValue)) {
+			case ValueType.TEnum(Directive):
+				Reflect.setField(this, name, DirectiveTools.fromDynamic(value));
+			case _:
+				super.configureProperty(name, value);
+		}
+	}
+
 	override function actualRun() {
 		var lastExpr = null;
 
@@ -50,18 +63,18 @@ class SpacingCheck extends Check {
 					if (post) dist = e.pos.max - e2.pos.max;
 					else dist = e2.pos.min - e.pos.min;
 					if (dist > unopSize(uo)) logPos('Space around "${unopString(uo)}"', e.pos);
-				case EIf(econd, _, _) if (spaceIfCondition):
-					checkSpaceBetweenExpressions("if", e, econd);
-				case EFor(it, _) if (spaceForLoop):
-					checkSpaceBetweenExpressions("for", e, it);
-				case EWhile(econd, _, true) if (spaceWhileLoop):
-					checkSpaceBetweenExpressions("while", e, econd);
-				case ESwitch(eswitch, _, _) if (spaceSwitchCase):
-					checkSpaceBetweenManually("switch", lastExpr, eswitch);
-				case ETry(etry, catches) if (spaceCatch):
+				case EIf(econd, _, _):
+					checkSpaceBetweenExpressions("if", e, econd, spaceIfCondition);
+				case EFor(it, _):
+					checkSpaceBetweenExpressions("for", e, it, spaceForLoop);
+				case EWhile(econd, _, true):
+					checkSpaceBetweenExpressions("while", e, econd, spaceWhileLoop);
+				case ESwitch(eswitch, _, _):
+					checkSpaceBetweenManually("switch", lastExpr, eswitch, spaceSwitchCase);
+				case ETry(etry, catches):
 					var exprBeforeCatch = lastExpr;
 					for (ctch in catches) {
-						checkSpaceBetweenManually("catch", exprBeforeCatch, ctch.expr);
+						checkSpaceBetweenManually("catch", exprBeforeCatch, ctch.expr, spaceCatch);
 						exprBeforeCatch = ctch.expr;
 					}
 				default:
@@ -87,18 +100,35 @@ class SpacingCheck extends Check {
 		return (new Printer()).printUnop(uo);
 	}
 
-	function checkSpaceBetweenExpressions(name:String, e1:Expr, e2:Expr) {
-		if (e2.pos.min - e1.pos.min < '$name ('.length) {
-			logRange('No space between "$name" and "("', e1.pos.max, e2.pos.min);
+	function checkSpaceBetweenExpressions(name:String, e1:Expr, e2:Expr, directive:Directive) {
+		switch (directive) {
+			case ANY:
+			case SHOULD_NOT:
+				if (e2.pos.min - e1.pos.min > '$name('.length) {
+					logRange('Space between "$name" and "("', e2.pos.max, e2.pos.min);
+				}
+			case SHOULD:
+				if (e2.pos.min - e1.pos.min < '$name ('.length) {
+					logRange('No space between "$name" and "("', e1.pos.max, e2.pos.min);
+				}
 		}
 	}
 
-	function checkSpaceBetweenManually(name:String, before:Expr, check:Expr) {
+	function checkSpaceBetweenManually(name:String, before:Expr, check:Expr, directive:Directive) {
 		var prevExprUntilChecked = checker.file.content.substring(before.pos.min, check.pos.min + 1);
 		var checkPos = prevExprUntilChecked.lastIndexOf('$name(');
-		if (checkPos > -1) {
-			var fileCheckPos = before.pos.min + checkPos;
-			logRange('No space between "$name" and "("', fileCheckPos, fileCheckPos + '$name('.length);
+		var fileCheckPos = before.pos.min + checkPos;
+
+		switch (directive) {
+			case ANY:
+			case SHOULD_NOT:
+				if (checkPos < 0) {
+					logRange('Space between "$name" and "("', check.pos.min, check.pos.max);
+				}
+			case SHOULD:
+				if (checkPos > -1) {
+					logRange('No space between "$name" and "("', fileCheckPos, fileCheckPos + '$name('.length);
+				}
 		}
 	}
 }

src/checkstyle/errors/Error.hx

@@ -0,0 +1,13 @@
+package checkstyle.errors;
+
+class Error {
+	public var message (default, null):String;
+
+	public function new(message:String) {
+		this.message = message;
+	}
+
+	public function toString():String {
+		return message;
+	}
+}

src/checkstyle/errors/InvalidDirectiveError.hx

@@ -0,0 +1,3 @@
+package checkstyle.errors;
+
+class InvalidDirectiveError extends Error {}

test/checks/whitespace/SpacingCheckTest.hx

@@ -1,14 +1,23 @@
 package checks.whitespace;
 
 import checkstyle.checks.whitespace.SpacingCheck;
+import checkstyle.checks.Directive;
 
 class SpacingCheckTest extends CheckTestCase<SpacingCheckTests> {
 
-	public function testIf() {
+	public function testIfShouldContainSpace() {
 		assertMsg(new SpacingCheck(), TEST1A, 'No space between "if" and "("');
 		assertNoMsg(new SpacingCheck(), TEST1B);
 	}
 
+	public function testIfShouldNotContainSpace() {
+		var check = new SpacingCheck();
+		check.spaceIfCondition = Directive.SHOULD_NOT;
+
+		assertMsg(check, TEST1B, 'Space between "if" and "("');
+		assertNoMsg(check, TEST1A);
+	}
+
 	public function testBinaryOperator() {
 		assertMsg(new SpacingCheck(), TEST2, 'No space around "+"');
 	}
@@ -17,25 +26,57 @@ class SpacingCheckTest extends CheckTestCase<SpacingCheckTests> {
 		assertMsg(new SpacingCheck(), TEST3, 'Space around "++"');
 	}
 
-	public function testFor() {
+	public function testForShouldContainSpace() {
 		assertMsg(new SpacingCheck(), TEST4A, 'No space between "for" and "("');
 		assertNoMsg(new SpacingCheck(), TEST4B);
 	}
 
-	public function testWhile() {
+	public function testForShouldNotContainSpace() {
+		var check = new SpacingCheck();
+		check.spaceForLoop = Directive.SHOULD_NOT;
+
+		assertMsg(check, TEST4B, 'Space between "for" and "("');
+		assertNoMsg(check, TEST4A);
+	}
+
+	public function testWhileShouldContainSpace() {
 		assertMsg(new SpacingCheck(), TEST5A, 'No space between "while" and "("');
 		assertNoMsg(new SpacingCheck(), TEST5B);
 	}
 
-	public function testSwitch() {
+	public function testWhileShouldNotContainSpace() {
+		var check = new SpacingCheck();
+		check.spaceWhileLoop = Directive.SHOULD_NOT;
+
+		assertMsg(check, TEST5B, 'Space between "while" and "("');
+		assertNoMsg(check, TEST5A);
+	}
+
+	public function testSwitchShouldContainSpace() {
 		assertMsg(new SpacingCheck(), TEST6A, 'No space between "switch" and "("');
 		assertNoMsg(new SpacingCheck(), TEST6B);
 	}
 
-	public function testCatch() {
+	public function testSwitchShouldNotContainSpace() {
+		var check = new SpacingCheck();
+		check.spaceSwitchCase = Directive.SHOULD_NOT;
+
+		assertMsg(check, TEST6B, 'Space between "switch" and "("');
+		assertNoMsg(check, TEST6A);
+	}
+
+	public function testCatchShouldContainSpace() {
 		assertMsg(new SpacingCheck(), TEST7A, 'No space between "catch" and "("');
 		assertNoMsg(new SpacingCheck(), TEST7B);
 	}
+
+	public function testCatchShouldNotContainSpace() {
+		var check = new SpacingCheck();
+		check.spaceCatch = Directive.SHOULD_NOT;
+
+		assertMsg(check, TEST7B, 'Space between "catch" and "("');
+		assertNoMsg(check, TEST7A);
+	}
 }
 
 @:enum
@@ -54,6 +95,7 @@ abstract SpacingCheckTests(String) to String {
 		}
 	}";
 
+
 	var TEST2 =
 	"class Test {
 		public function test() {