Add style and static analyzer checks #301

dzubchik · 2017-11-27T21:34:46Z

I've added fixes after analyzer, so please review them. I propose to choose rules from the list.

dzubchik · 2017-11-28T09:03:15Z

composer.json

-        "phpunit/phpunit": "^5.7"
+        "phpunit/phpunit": "^5.7",
+        "friendsofphp/php-cs-fixer": "^2.8",
+        "phpstan/phpstan": "^0.8.5"


remove from dependencies until 7.0. Require global on CI

dzubchik · 2017-11-30T21:28:30Z

@andig , @kelunik , @marcj ping

kelunik · 2017-11-30T21:33:30Z

.phpstan.neon

+    excludes_analyse:
+        - %rootDir%/vendor/*
+    ignoreErrors:
+        - '#Result of method React\\Socket\\ServerInterface::close\(\) \(void\) is used#'


Why is this ignored?

Because of usage "@" https://github.com/php-pm/php-pm/blob/master/ProcessManager.php#L238

kelunik · 2017-11-30T21:34:24Z

.travis.yml

@@ -1,3 +1,4 @@
+---


$ yamllint .travis.yml 1:1 warning missing document start "---" (document-start) 14:81 error line too long (189 > 80 characters) (line-length) 16:81 error line too long (161 > 80 characters) (line-length) 17:81 error line too long (254 > 80 characters) (line-length) 31:81 error line too long (95 > 80 characters) (line-length)

Remove?

andig · 2017-12-03T10:56:39Z

.travis.yml

  - rm composer.lock && rm -rf vendor
  - composer install
  - ./vendor/bin/phpunit

+  # composer can not install httpkernel-adapter with default stability
+  - composer config minimum-stability dev


Why is this needed? We currently are on dev stability?

andig · 2017-12-03T10:58:02Z

Client.php

@@ -4,8 +4,9 @@

 use React\EventLoop\Factory;
 use React\EventLoop\LoopInterface;
-use React\Socket\UnixConnector;
+use React\Promise\ExtendedPromiseInterface;


Should be PromiseInterface?

andig · 2017-12-03T10:58:09Z

Client.php

@@ -27,7 +28,7 @@ public function __construct()
    }

    /**
-     * @return ConnectionInterface
+     * @return ExtendedPromiseInterface


Should be PromiseInterface?

andig · 2017-12-03T10:59:56Z

I'd suggest getting this in right after #305 which already has a number of clean-ups.

dzubchik · 2017-12-13T21:03:22Z

@andig , @kelunik any comments?

dzubchik · 2017-12-13T21:06:05Z

One more note: I have not added phpstan and php-cs-fixer to require-dev section because of php5.6. So, now you should have globally installed phpstan && php-cs-fixer.

dzubchik · 2017-12-19T07:54:12Z

Any feedback?

andig · 2017-12-19T21:12:05Z

Any feedback?

Sorry for the troubles. Could you please recheck? I still see the odd ExtendedPromiseInterface which need be removed?

dzubchik · 2017-12-20T16:11:05Z

I've updated PR according to latest refactoring with slaves. Removed ExtendedPromiseInterface, because that part of the code has been removed.

dzubchik · 2018-01-09T07:16:05Z

@andig , @marcj what wrong with this PR? It seemed that this PR is important according to 2 new PR opened for last day. Are any chances for it to be merged after one more rebase?

andig · 2018-01-09T16:21:11Z

@dzubchik sorry for the delay- I didn't notice you had pushed once more.

I'm happy with all the changes except for the ci updates. It seems we have 3 code analysis tools running now and I'm lost on how to of which one to integrate (and have it hint or fail). Funny enough they also come up with different findings :)

I'd propose to merge the code updates but not change the CI pipeline for now? And then lets merge them asap to not get bit-rotten again.

szepeviktor · 2018-01-09T19:55:42Z

Funny enough they also come up with different findings :)

This is a good reason to keep all of them around!

szepeviktor · 2018-01-09T19:57:07Z

It seems we have 3 code analysis tools running now

And two of them are called "phpstan" 😃

dzubchik · 2018-01-10T20:07:06Z

@andig PR is updated. Let me know if something should be changed.

andig · 2018-01-10T20:47:50Z

@dzubchik as I wrote- I like the findings but

I'd propose to merge the code updates but not change the CI pipeline for now?

Could you remove the phpstan config and travis.yml changes?

dzubchik · 2018-01-11T09:17:01Z

@andig , phpstan.neon config can not be removed because it allows to ignore these:

 ------ ------------------------------------------------------------------------ 
  Line   src/ProcessSlave.php                                                    
 ------ ------------------------------------------------------------------------ 
  200    Result of method React\Socket\ServerInterface::close() (void) is used.  
 ------ ------------------------------------------------------------------------ 
  Line   src/ProcessManager.php                                                  
 ------ ------------------------------------------------------------------------ 
  218    Result of method React\Socket\ServerInterface::close() (void) is used.  
  221    Result of method React\Socket\ServerInterface::close() (void) is used.

These "errors" appeared because of "@" usage.

And the .travis.yml must have these checks otherwise it does not make sense. If we do not run each build this, we should not add these tools at all. And now phpstan and php-code-fixer are not in "require-dev" section because of php5.6. After upgrading we can add them to "composer.json" and developer will have these tools during development. Now, he should install phpstan and php-cs-fixer globally to use them.

andig · 2018-01-11T10:09:39Z

.travis.yml

+  # Static analyzer check
+  - if [ $TRAVIS_PHP_VERSION = '7.1' ]; then composer require "phpstan/phpstan:^0.9" && ./vendor/bin/phpstan analyze -c .phpstan.neon --level=2 --no-progress src;fi
+  # Check the code style
+  - if [ $TRAVIS_PHP_VERSION = '7.1' ]; then IFS=$'\n'; COMMIT_SCA_FILES=($(git diff --name-only --diff-filter=ACMRTUXB "${TRAVIS_COMMIT_RANGE}")); unset IFS; fi


Why is this needed?

This added to run static analyze checks and code style checks, so code with wrong style-guide, some misspelling, incorrect return types and phpdocs will not be merged to master branch. And it should be run once, not for each php version. The instruction for php-cs-fixer was taken from documentation.

andig · 2018-01-11T11:14:26Z

Great, much appreciated and thank you for your patience!

dzubchik commented Nov 28, 2017

View reviewed changes

dzubchik force-pushed the master branch from 7d06982 to 7e45095 Compare November 30, 2017 19:38

dzubchik changed the title ~~[wip] add style and static analyzer checks~~ add style and static analyzer checks Nov 30, 2017

dzubchik force-pushed the master branch from 9c4fb48 to 2367869 Compare November 30, 2017 21:24

dzubchik changed the title ~~add style and static analyzer checks~~ Add style and static analyzer checks Nov 30, 2017

dzubchik force-pushed the master branch 2 times, most recently from 30991fc to 1d018a9 Compare November 30, 2017 21:30

kelunik reviewed Nov 30, 2017

View reviewed changes

andig reviewed Dec 3, 2017

View reviewed changes

andig requested changes Dec 3, 2017

View reviewed changes

dzubchik force-pushed the master branch 2 times, most recently from 6a3f886 to b3f9cba Compare December 13, 2017 20:23

dzubchik changed the title ~~Add style and static analyzer checks~~ [wip] Add style and static analyzer checks Dec 13, 2017

dzubchik force-pushed the master branch 2 times, most recently from b8f72c2 to 5fd8569 Compare December 13, 2017 20:58

dzubchik changed the title ~~[wip] Add style and static analyzer checks~~ Add style and static analyzer checks Dec 13, 2017

dzubchik force-pushed the master branch 2 times, most recently from c7e4d92 to 1e78682 Compare December 20, 2017 15:52

andig mentioned this pull request Dec 22, 2017

RFC: Roadmap #297

Closed

15 tasks

dzubchik mentioned this pull request Jan 9, 2018

Introduce phpstan for free #329

Closed

Rebase

2420fae

dzubchik force-pushed the master branch from 1e78682 to 2420fae Compare January 10, 2018 19:52

Update configs

a7ca9ae

andig reviewed Jan 11, 2018

View reviewed changes

andig merged commit 3328fca into php-pm:master Jan 11, 2018

andig mentioned this pull request Feb 4, 2018

PR build broken #352

Closed

Add style and static analyzer checks #301

Add style and static analyzer checks #301

Uh oh!

Conversation

dzubchik commented Nov 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzubchik commented Nov 30, 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!

andig commented Dec 3, 2017

Uh oh!

dzubchik commented Dec 13, 2017

Uh oh!

dzubchik commented Dec 13, 2017

Uh oh!

dzubchik commented Dec 19, 2017

Uh oh!

andig commented Dec 19, 2017

Uh oh!

dzubchik commented Dec 20, 2017

Uh oh!

dzubchik commented Jan 9, 2018

Uh oh!

andig commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szepeviktor commented Jan 9, 2018

Uh oh!

szepeviktor commented Jan 9, 2018

Uh oh!

dzubchik commented Jan 10, 2018

Uh oh!

andig commented Jan 10, 2018

Uh oh!

dzubchik commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andig commented Jan 11, 2018

Uh oh!

Uh oh!

andig commented Jan 9, 2018 •

edited

Loading

dzubchik commented Jan 11, 2018 •

edited

Loading