captainhook icon indicating copy to clipboard operation
captainhook copied to clipboard

If php-parallel-lint is installed, the shipped "Linting" action should use it

Open kingkero opened this issue 1 year ago • 1 comments
trafficstars

Currently, the CaptainHook\App\Hook\PHP\Action\Linting action uses

        $result  = $process->run($this->php . ' -l ' . escapeshellarg($file));

As this is called once per file, it can be slow for large commits. If the project already contains php-parallel-lint, the action could use this to lint all files in a single execution in parallel (and fallback to the current implementation if the package is not installed).

It should be simple and cheap to check for installed packages via \Composer\InstalledVersions::isInstalled('foo/bar') (see https://getcomposer.org/doc/07-runtime.md#installed-versions).

Since php -l also accepts multiple arguments, one could pass all files instead of doing a single call per file. In my test, the difference was that php -l a.php b.php will not fail early. So even if a.php is invalid, it will check b.php ...

kingkero avatar Aug 30 '24 13:08 kingkero

I think I would prefer a separate Action for parallel linting. I don't like the idea of using Composer to detect if something is installed. That would complicate the PHAR build an introduce dependencies to composer.json and so on.

If you can detect it without using Composer I'm all for it. An alternative could be to add an Option to the Action (eg linter => path/to/php-parallel-lint) so it becomes an active choice.

But I think I would prefer the own Action strategy if you can't reliably detect it without using Composer classes.

sebastianfeldmann avatar Aug 30 '24 13:08 sebastianfeldmann

or use this

{
  "action": "vendor/bin/php-parallel-lint {$CHANGED_FILES}"
}

sebastianfeldmann avatar Sep 01 '24 18:09 sebastianfeldmann