RequireTernaryOperatorSniff: Fixer should not remove comments

Author: kukulich

SlevomatCodingStandard/Sniffs/ControlStructures/RequireTernaryOperatorSniff.php

@@ -4,6 +4,7 @@
 
 use PHP_CodeSniffer\Files\File;
 use PHP_CodeSniffer\Sniffs\Sniff;
+use PHP_CodeSniffer\Util\Tokens;
 use SlevomatCodingStandard\Helpers\IdentificatorHelper;
 use SlevomatCodingStandard\Helpers\TokenHelper;
 use function array_key_exists;
@@ -81,7 +82,21 @@ public function process(File $phpcsFile, $ifPointer): void
 
 	private function checkIfWithReturns(File $phpcsFile, int $ifPointer, int $elsePointer, int $returnInIf, int $returnInElse): void
 	{
-		$fix = $phpcsFile->addFixableError('Use ternary operator.', $ifPointer, self::CODE_TERNARY_OPERATOR_NOT_USED);
+		$ifContainsComment = $this->containsComment($phpcsFile, $ifPointer);
+		$elseContainsComment = $this->containsComment($phpcsFile, $elsePointer);
+
+		$errorParameters = [
+			'Use ternary operator.',
+			$ifPointer,
+			self::CODE_TERNARY_OPERATOR_NOT_USED,
+		];
+
+		if ($ifContainsComment || $elseContainsComment) {
+			$phpcsFile->addError(...$errorParameters);
+			return;
+		}
+
+		$fix = $phpcsFile->addFixableError(...$errorParameters);
 
 		if (!$fix) {
 			return;
@@ -157,7 +172,21 @@ private function checkIfWithAssignments(
 			return;
 		}
 
-		$fix = $phpcsFile->addFixableError('Use ternary operator.', $ifPointer, self::CODE_TERNARY_OPERATOR_NOT_USED);
+		$ifContainsComment = $this->containsComment($phpcsFile, $ifPointer);
+		$elseContainsComment = $this->containsComment($phpcsFile, $elsePointer);
+
+		$errorParameters = [
+			'Use ternary operator.',
+			$ifPointer,
+			self::CODE_TERNARY_OPERATOR_NOT_USED,
+		];
+
+		if ($ifContainsComment || $elseContainsComment) {
+			$phpcsFile->addError(...$errorParameters);
+			return;
+		}
+
+		$fix = $phpcsFile->addFixableError(...$errorParameters);
 
 		if (!$fix) {
 			return;
@@ -213,4 +242,15 @@ private function isCompatibleScope(File $phpcsFile, int $scopeOpenerPointer, int
 		return $pointerAfterSemicolon === $scopeCloserPointer;
 	}
 
+	private function containsComment(File $phpcsFile, int $scopeOwnerPointer): bool
+	{
+		$tokens = $phpcsFile->getTokens();
+		return TokenHelper::findNext(
+			$phpcsFile,
+			Tokens::$commentTokens,
+			$tokens[$scopeOwnerPointer]['scope_opener'] + 1,
+			$tokens[$scopeOwnerPointer]['scope_closer']
+		) !== null;
+	}
+
 }

tests/Sniffs/ControlStructures/RequireTernaryOperatorSniffTest.php

@@ -16,12 +16,16 @@ public function testErrors(): void
 	{
 		$report = self::checkFile(__DIR__ . '/data/requireTernaryOperatorErrors.php');
 
-		self::assertSame(4, $report->getErrorCount());
+		self::assertSame(8, $report->getErrorCount());
 
 		self::assertSniffError($report, 4, RequireTernaryOperatorSniff::CODE_TERNARY_OPERATOR_NOT_USED);
-		self::assertSniffError($report, 11, RequireTernaryOperatorSniff::CODE_TERNARY_OPERATOR_NOT_USED);
-		self::assertSniffError($report, 22, RequireTernaryOperatorSniff::CODE_TERNARY_OPERATOR_NOT_USED);
-		self::assertSniffError($report, 31, RequireTernaryOperatorSniff::CODE_TERNARY_OPERATOR_NOT_USED);
+		self::assertSniffError($report, 12, RequireTernaryOperatorSniff::CODE_TERNARY_OPERATOR_NOT_USED);
+		self::assertSniffError($report, 21, RequireTernaryOperatorSniff::CODE_TERNARY_OPERATOR_NOT_USED);
+		self::assertSniffError($report, 29, RequireTernaryOperatorSniff::CODE_TERNARY_OPERATOR_NOT_USED);
+		self::assertSniffError($report, 35, RequireTernaryOperatorSniff::CODE_TERNARY_OPERATOR_NOT_USED);
+		self::assertSniffError($report, 42, RequireTernaryOperatorSniff::CODE_TERNARY_OPERATOR_NOT_USED);
+		self::assertSniffError($report, 54, RequireTernaryOperatorSniff::CODE_TERNARY_OPERATOR_NOT_USED);
+		self::assertSniffError($report, 63, RequireTernaryOperatorSniff::CODE_TERNARY_OPERATOR_NOT_USED);
 
 		self::assertAllFixedInFile($report);
 	}

tests/Sniffs/ControlStructures/data/requireTernaryOperatorErrors.fixed.php

@@ -4,8 +4,40 @@ function () {
 	return true ? true : false;
 };
 
+function () {
+	if (true) {
+		// Comment
+		return true;
+	} else {
+		return false;
+	}
+};
+
+function () {
+	if (true) {
+		return true;
+	} else {
+		// Comment
+		return false;
+	}
+};
+
 $a = doSomething() ? 'a' : 'aa';
 
+if (doAnything()) {
+	// Comment
+	$a = 'a';
+} else {
+	$a = 'aa';
+}
+
+if (doNothing()) {
+	$a = 'a';
+} else {
+	// Comment
+	$a = 'aa';
+}
+
 class Whatever
 {
 

tests/Sniffs/ControlStructures/data/requireTernaryOperatorErrors.php

@@ -8,12 +8,44 @@ function () {
 	}
 };
 
+function () {
+	if (true) {
+		// Comment
+		return true;
+	} else {
+		return false;
+	}
+};
+
+function () {
+	if (true) {
+		return true;
+	} else {
+		// Comment
+		return false;
+	}
+};
+
 if (doSomething()) {
 	$a = 'a';
 } else {
 	$a = 'aa';
 }
 
+if (doAnything()) {
+	// Comment
+	$a = 'a';
+} else {
+	$a = 'aa';
+}
+
+if (doNothing()) {
+	$a = 'a';
+} else {
+	// Comment
+	$a = 'aa';
+}
+
 class Whatever
 {