Implement missing sniffs (upgrading to PHPCS v3) #8

lcobucci · 2017-08-19T23:32:21Z

Making sure we have all sniffs we use on doctrine projects.

Replaces #7

lcobucci · 2017-08-19T23:35:40Z

Still need to update documentation, sure...

Ocramius · 2017-08-19T23:37:19Z

.travis.yml

+php:
+  - 7.1
+  - 7.2
+  - nightly


Nightly should be allowed to fail

Ocramius · 2017-08-19T23:37:47Z

composer.json

+    "require": {
+        "php": "^7.1",
+        "squizlabs/php_codesniffer": "~3.0",
+        "dealerdirect/phpcodesniffer-composer-installer": "^0.4"


O_o

Also - unsure if I'd want that in the dep chain...

The other option is to manually have composer scripts to register the directory (on each repo).

Ocramius · 2017-08-19T23:38:08Z

lib/Doctrine/Sniffs/Spacing/ControlStructureSniff.php

+    public function register()
+    {
+        return [
+            T_IF,


Use FQNs for absolute symbols (prefix with \)

Ocramius · 2017-08-19T23:38:35Z

lib/Doctrine/Sniffs/Spacing/ControlStructureSniff.php

+    {
+        $tokens = $phpcsFile->getTokens();
+
+        if (isset($tokens[$stackPtr]['parenthesis_opener']) === false


Is there a constant for these keys?

Nope... I can add.

Ocramius · 2017-08-19T23:38:49Z

lib/Doctrine/Sniffs/Spacing/ControlStructureSniff.php

+        ];
+    }
+
+    public function process(File $phpcsFile, $stackPtr)


What's the method return type?

According to the interface is void|int, so no way to add the return type declaration here.

Ocramius · 2017-08-19T23:39:16Z

lib/Doctrine/Sniffs/Spacing/ControlStructureSniff.php

+        $file->fixer->replaceToken($nextTokenPosition, '');
+    }
+
+    private function validateParenthesisCloser(


Please document the array parameter type

That comes from PHP_CodeSniffer\Files\File#tokens and all it says is: "The tokens stack map.". I'm not sure what we should add here.

I can add mixed[][] if it helps 😂

Ocramius · 2017-08-19T23:39:51Z

lib/Doctrine/Sniffs/Spacing/SpaceOnNotSniff.php

-            31 => 1,
-            34 => 1,
-        );
+        return [T_BOOLEAN_NOT];


Generally, look for T_ and use the FQN

Ocramius · 2017-08-19T23:40:23Z

lib/Doctrine/Sniffs/Spacing/SpaceOnReturnTypeSniff.php

-            $phpcsFile->addError('Missing space before the string concatenation operator ".".', $stackPtr, 'Before');
-        }
+        do {
+            --$colonPosition;


Use array_reverse() instead of relying on decrements - really odd and uncommon to read

Can't reverse the entire map of tokens, maybe extracting that into a method with a proper name?

Ah, I see - you basically need that position to be set after the while.

Is it possible that this block ever goes into an infinite loop? It looks like it, but the assumptions around its design are missing for me.

That sniff is applied on return type tokens, there's no way to declare return types without using the colon. It might go into an infinite loop if PHP syntax changes.

Got it 👍

Ocramius · 2017-08-19T23:40:37Z

lib/Doctrine/ruleset.xml

@@ -0,0 +1,33 @@
+<?xml version="1.0"?>
+<ruleset name="Doctrine">


Is there an XSD somewhere?

Ocramius · 2017-08-19T23:40:51Z

phpcs.xml.dist

+    <arg name="basepath" value="."/>
+    <arg name="extensions" value="php"/>
+    <arg name="colors" />
+    <arg value="p"/>


Progress: https://travis-ci.org/doctrine/coding-standard/jobs/266394932#L474

Comment plox

Ocramius · 2017-08-20T00:18:04Z

lib/Doctrine/Sniffs/Spacing/SpaceOnReturnTypeSniff.php

-            $phpcsFile->addError('Missing space before the string concatenation operator ".".', $stackPtr, 'Before');
-        }
+        do {
+            --$colonPosition;


Ah, I see - you basically need that position to be set after the while.

Is it possible that this block ever goes into an infinite loop? It looks like it, but the assumptions around its design are missing for me.

Ocramius · 2017-08-20T00:18:17Z

lib/Doctrine/ruleset.xml

@@ -0,0 +1,33 @@
+<?xml version="1.0"?>
+<ruleset name="Doctrine">


Ocramius · 2017-08-20T00:18:22Z

phpcs.xml.dist

+    <arg name="basepath" value="."/>
+    <arg name="extensions" value="php"/>
+    <arg name="colors" />
+    <arg value="p"/>


Comment plox

Since we have a better option in PHPCS v3

We were missing the checks for: - Spaces before AND after not operator - Spaces before AND after return type declaration

So that our standard gets automatically registered to PHPCS.

That's easier than doing some workarounds on PHPUnit.

lcobucci added the Improvement label Aug 19, 2017

lcobucci self-assigned this Aug 19, 2017

lcobucci requested a review from Ocramius August 19, 2017 23:32

lcobucci force-pushed the upgrade-dependencies branch from 07068be to 4fa08c1 Compare August 19, 2017 23:33

Ocramius suggested changes Aug 19, 2017

View reviewed changes

lcobucci force-pushed the upgrade-dependencies branch from 4fa08c1 to b340846 Compare August 20, 2017 00:12

lcobucci requested a review from Ocramius August 20, 2017 00:15

Ocramius suggested changes Aug 20, 2017

View reviewed changes

lcobucci added 6 commits August 20, 2017 02:23

Remove old sniff

e76a9f0

Since we have a better option in PHPCS v3

Remove PHPUnit since it's not really necessary

da97bc3

Add new sniffs to validate new rules

aedfc60

We were missing the checks for: - Spaces before AND after not operator - Spaces before AND after return type declaration

Use dealerdirect/phpcodesniffer-composer-installer

4bd6c9b

So that our standard gets automatically registered to PHPCS.

Use PHPCS itself to validate our sniffs

cdce50d

That's easier than doing some workarounds on PHPUnit.

Apply necessary modifications to build

80deebe

lcobucci force-pushed the upgrade-dependencies branch from b340846 to 80deebe Compare August 20, 2017 00:23

Update documentation

20decd8

lcobucci requested a review from Ocramius August 20, 2017 00:47

Ocramius added this to the 1.0.0 milestone Aug 20, 2017

Ocramius approved these changes Aug 20, 2017

View reviewed changes

Ocramius merged commit 50ef313 into master Aug 20, 2017

Ocramius deleted the upgrade-dependencies branch August 20, 2017 00:50

lcobucci mentioned this pull request Aug 20, 2017

[WIP] Imported logical NOT rule from "doctrine/doctrine2" #7

Closed

2 tasks

lcobucci changed the title ~~Upgrade dependencies (PHPCS v3)~~ Implement missing sniffs (upgrading to PHPCS v3) Aug 20, 2017

		@@ -0,0 +1,33 @@
		<?xml version="1.0"?>
		<ruleset name="Doctrine">

Implement missing sniffs (upgrading to PHPCS v3) #8

Implement missing sniffs (upgrading to PHPCS v3) #8

Uh oh!

Conversation

lcobucci commented Aug 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lcobucci commented Aug 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lcobucci commented Aug 19, 2017 •

edited

Loading