phpro / grumphp

A PHP code-quality tool
MIT License
4.11k stars 429 forks source link

husky/lint-staged compatibility #1127

Closed AlecRust closed 2 weeks ago

AlecRust commented 3 months ago
Q A
Version 2.5.0
Bug? unsure
New feature? yes
Question? yes
Documentation? yes

I use husky and lint-staged in my WordPress plugin to e.g. run Prettier to format my PHP code on commit.

I've added GrumPHP via Composer to handle PHP-related linting tasks. Since GrumPHP also looks to "sniff your commits" I assumed GrumPHP would run on the staged files during commit, but this does not seem to happen - only the husky hooks run.

Is there any way you know of to work around this issue, or have you considered native compatibility with husky/lint-staged?

I did try running GrumPHP through lint-staged like I do Prettier:

.lintstagedrc.json

{
    "*": "prettier --write --ignore-unknown",
    "*.php": "./vendor/bin/grumphp run"
}

But it appears you can't pass an argument to the GrumPHP run command, in this case the path(s) of the staged files.

Another option I considered was running Prettier through GrumPHP using npm_script:

grumphp.yml

parameters:
    tasks:
        npm_script:
            script: node_modules/.bin/prettier --check .

But the check never seems to complete with . Any ideas?

My configuration

# grumphp.yml
parameters:
    tasks:
        phpparser:
            visitors:
                forbidden_function_calls:
                    blacklist:
                        - "var_dump"
                        - "print_r"
        phpversion:
            project: "7.0"
        phpcs:
            standard: PSR2
        phpmd:
            ruleset: ["cleancode", "naming"]
        phpmnd:
        phpstan:
            memory_limit: 512M

Steps to reproduce:

  1. Setup repository with husky and lint-staged e.g. to run Prettier on commit
  2. Setup GrumPHP and tasks for it to run
  3. On commit, no GrumPHP Git hooks occur, but lint-staged hooks do
veewee commented 3 months ago

Hello,

It depends which tool you want to make leading I suppose.

I don't know the tool lint-staged, but there might be 2 options:

You could indeed make grumphp leading as well and add a custom bash script. I don't know why the command does not return an exit code of 0, but that's probably more related to the tool than to grumphp.

AlecRust commented 3 months ago

Thanks for your response!

It depends which tool you want to make leading I suppose.

Whichever one will allow me to run both Prettier and GrumPHP 🙂 feels like either approach is close really - I can "almost" run Prettier as a GrumPHP npm task, and I can "almost" run GrumPHP via lint-staged.

the run command takes a list of files on STDIN which could be used

This would be great. Is there any reason GrumPHP currently can't be given paths to run on?

You could use the git:pre-commit command which uses the changed files only

Great idea, thanks. Sadly this doesn't work though as lint-staged still passes the path which causes an error:

[STARTED] Preparing lint-staged...
[COMPLETED] Preparing lint-staged...
[STARTED] Running tasks for staged files...
[STARTED] .lintstagedrc.json — 2 files
[STARTED] * — 2 files
[STARTED] *.php — 1 file
[STARTED] prettier --write --ignore-unknown
[STARTED] ./vendor/bin/grumphp git:pre-commit
[FAILED] ./vendor/bin/grumphp git:pre-commit [FAILED]
[FAILED] ./vendor/bin/grumphp git:pre-commit [FAILED]
[COMPLETED] Running tasks for staged files...
[STARTED] Applying modifications from tasks...
[SKIPPED] Skipped because of errors from tasks.
[STARTED] Reverting to original state because of errors...
[FAILED] prettier --write --ignore-unknown [KILLED]
[FAILED] prettier --write --ignore-unknown [KILLED]
[COMPLETED] Reverting to original state because of errors...
[STARTED] Cleaning up temporary files...
[COMPLETED] Cleaning up temporary files...

✖ ./vendor/bin/grumphp git:pre-commit:

  No arguments expected for "git:pre-commit" command, got "/Users/alec/projec  
  ts/personal/brevwoo/includes/admin.php".  

I don't know why the command does not return an exit code of 0

I'm pretty sure Prettier CLI does return an exit code of 0 when it succeeds. In fact even this npm script doesn't work:

parameters:
    tasks:
        npm_script:
            script: "echo 'Hello World'; exit 0"

Which leads me to think npm script tasks aren't working in GrumPHP at all.

veewee commented 3 months ago

Which leads me to think npm script tasks aren't working in GrumPHP at all.

You are basically running:

 npm run "echo 'Hello World'; exit 0"

Which I dont think is supported. You should run a script that is defined in package.json. For exampe:

npm run prettier

when the package.json contains:

{
    "scripts": {
        "prettier": "prettier --check"
    }
}

Next, you need to configure grumphp to execute a nmp run task:

grumphp:
    tasks:
        prettier:
            script: "prettier"
            triggered_by: [js, jsx, coffee, ts, less, sass, scss]
            working_directory: "./"
            is_run_task: true
            silent: false
            metadata:
                task: npm_script

If you just want to run a bash command, you need the shell task.

AlecRust commented 3 months ago

Ah I see, that makes sense thanks. However in my testing even that is not working 😅

  "scripts": {
    "prettier": "prettier --check ."
  }
parameters:
    tasks:
        prettier:
            script: "prettier"
            triggered_by: [js, jsx, coffee, ts, less, sass, scss]
            working_directory: "./"
            is_run_task: true
            silent: false
            metadata:
                task: npm_script
Running tasks with priority 0!
==============================

Running task 1/7: prettier...
Running task 2/7: phpparser... ✔
Running task 3/7: phpversion... ✔
Running task 4/7: phpcs... ✔
Running task 5/7: phpmd... ✔
Running task 6/7: phpmnd... ✔
Running task 7/7: phpstan... ✔

(same with and without Prettier check errors)

Besides, even if I was able to run Prettier through GrumPHP, it might simplify e.g. CI but it doesn't solve the GrumPHP pre-commit hook not running.

My current lint-staged setup runs all staged files through Prettier on commit. It would be nice to be able to include GrumPHP in that list for *.php files.

The alternative of removing husky/lint-staged and relying on GrumPHP for commit hooks isn't as appealing. Doesn't look like I would be able to e.g. run only all staged files though Prettier. I think I'd need to run a full prettier --write . on every commit.

veewee commented 2 weeks ago

Closing this issue for now. Feel free to provide a fix if you can come up with something.