captainhookphp / captainhook

CaptainHook is a very flexible git hook manager for software developers that makes sharing git hooks with your team a breeze.
http://captainhook.info
MIT License
996 stars 86 forks source link

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

Closed kingkero closed 4 weeks ago

kingkero commented 1 month ago

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 ...

sebastianfeldmann commented 1 month ago

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 commented 1 month ago

or use this

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