Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add PHPStan strict rules #638

Merged
merged 1 commit into from
May 6, 2018
Merged

Conversation

Majkl578
Copy link
Contributor

@Majkl578 Majkl578 commented May 6, 2018

No description provided.

@Majkl578 Majkl578 added this to the 2.0 milestone May 6, 2018
@Majkl578 Majkl578 requested review from jwage and alcaeus May 6, 2018 02:59
@Majkl578 Majkl578 force-pushed the phpstan-strict-rules branch from f3ee613 to b820b15 Compare May 6, 2018 03:28
@@ -24,7 +24,8 @@
"jdorn/sql-formatter": "~1.1",
"mikey179/vfsStream": "^1.6",
"phpstan/phpstan": "^0.9.2",
"symfony/process": "^4.0"
"symfony/process": "^4.0",
"phpstan/phpstan-strict-rules": "^0.9"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -556,7 +556,7 @@ public function getMigrationsToExecute(string $direction, string $to) : array
$this->loadMigrationsFromDirectory();

if ($direction === Version::DIRECTION_DOWN) {
if (count($this->migrations)) {
if (count($this->migrations) !== 0) {
Copy link
Member

@lcobucci lcobucci May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this might be a premature optimisation and that doctrine/migrations is not really sensitive regarding performance, but comparing with an empty array is faster than using count - even with PHP 7.2 optimisations - so I'd suggest to use if ($this->migrations !== []) { instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple bench:

<?php

namespace Blah;

use function assert;
use function count;

/**
 * @BeforeMethods({"init"})
 */
final class ArrayBench
{
    private $data;

    public function init(): void
    {
        $this->data = array_fill(0, 10000000, 'banana');
    }

    /**
     * @Revs(1000)
     * @Iterations(5)
     */
    public function benchCount()
    {
        assert(count($this->data) !== 0);
    }

    /**
     * @Revs(1000)
     * @Iterations(5)
     */
    public function benchComparison()
    {
        assert($this->data !== []);
    }
}

Result:

PhpBench 0.14-dev (60dfc50). Running benchmarks.
Using configuration file: /Users/luis/projects/chimera/bus-tactician/blah/phpbench.json

\Blah\ArrayBench

    benchCount                    I4 P0 	[μ Mo]/r: 0.092 0.092 (μs) 	[μSD μRSD]/r: 0.001μs 0.69%
    benchComparison               I4 P0 	[μ Mo]/r: 0.089 0.089 (μs) 	[μSD μRSD]/r: 0.000μs 0.55%

2 subjects, 10 iterations, 2,000 revs, 0 rejects
(best [mean mode] worst) = 0.088 [0.090 0.090] 0.089 (μs)
⅀T: 0.903μs μSD/r 0.001μs μRSD/r: 0.620%

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously that the size of the array and the data it contains would also affect the benchmark.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Majkl578 I'll merge it now so we can discuss this and eventually change (or not)

Copy link
Contributor Author

@Majkl578 Majkl578 May 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had discussion about this few days ago on Slack.
The difference is so minimal that I decided to ignore it completely. (Tens of miliseconds on 100 milion operations.)
Approach with count() is more future proof, if we eventually decide to introduce i.e. a) first class collections or b) php-ds.
The difference is not by factor of 100, not even by 100, in fact it's like 5%-10%. Using ! is much safer but that takes us to non-strict land and we don't want to go that path,

See https://gist.github.com/Majkl578/8fd1dec6b9e005e53c247a4953ccebbf

Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ I can't wait to have PHPStan v0.10 stable

@lcobucci lcobucci self-assigned this May 6, 2018
@lcobucci lcobucci merged commit c469ce1 into doctrine:master May 6, 2018
@lcobucci
Copy link
Member

lcobucci commented May 6, 2018

@Majkl578 🚢

@Majkl578 Majkl578 deleted the phpstan-strict-rules branch May 6, 2018 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants