search

astral-sh / ruff-pre-commit

A pre-commit hook for Ruff.
Apache License 2.0
802 stars 38 forks source link

re-staging after formatting #85

Closed Faithfinder closed 2 months ago

Faithfinder commented 3 months ago

It seems that pre-commit doesn't re-stage files that are changed, making auto-fixes and formatting in pre-commit, while not useless, a lot less ergonomic. Could this be handled on the hook's side?

dhruvmanila commented 2 months ago

I don't think that's possible because the hook only defines how to run the command and does not control any part of pre-commit or git.

I'll close this issue as it's not actionable via ruff-pre-commit. Happy to re-open if there's new information on this.

Faithfinder commented 2 months ago

I mean, technically nothing stops you from invoking git in the hook via subprocess.run or something, but I understand if you'd consider this out of scope. They do do that in JS world.

TizzySaurus commented 2 days ago

I don't think that's possible because the hook only defines how to run the command and does not control any part of pre-commit or git.

I'll close this issue as it's not actionable via ruff-pre-commit. Happy to re-open if there's new information on this.

@dhruvmanila Having something as simple as git add -u && !! (re-stage all modified files that are already staged [i.e. the files changed by pre-commit hook] and re-run the one-before-last command [which will be the original git commit]) would allow a more seamless pre-commit integration, even if it required passing an extra flag to the ruff pre-commit hook in the .pre-commit-config.yaml and so wasn't enabled by default.

Having worked in the JS ecosystem, it feels very 'mundane' to have to manually do this when the pre-commit hook can quite easily handle this for the developer (as it does in the JS ecosystem).

dhruvmanila commented 2 days ago

@dhruvmanila Having something as simple as git add -u && !! (re-stage all modified files that are already staged [i.e. the files changed by pre-commit hook] and re-run the one-before-last command [which will be the original git commit]) would allow a more seamless pre-commit integration, even if it required passing an extra flag to the ruff pre-commit hook in the .pre-commit-config.yaml and so wasn't enabled by default.

I'm not sure if I'd want to include additional commands (other than ruff) in the pre-commit hooks although other people on the team might have different opinion. There would be no way to not do that if one wishes to do so. Everyone would by default be opted-in to this feature. There's also the fact that in CI the pre-commit check should fail but a hook re-stages the changes files and then the command passes, there's no way the PR author will know about that.

Having worked in the JS ecosystem, it feels very 'mundane' to have to manually do this when the pre-commit hook can quite easily handle this for the developer (as it does in the JS ecosystem).

Can you please elaborate on what happens in the JS ecosystem? I'm not aware of it as I haven't worked that much in the ecosystem.

TizzySaurus commented 2 days ago

There would be no way to not do that if one wishes to do so. Everyone would by default be opted-in to this feature.

Is it not possible to add 'CLI-eque' arguments to the pre-commit hook, like you have with the ruff check and ruff format commands (something to go in .pre-commit-config.yaml)? If it is, then you can have a --restage-and-commit flag, or akin, that's disabled by default but users can optionally enable.

There's also the fact that in CI the pre-commit check should fail but a hook re-stages the changes files and then the command passes

The re-staging and committing will only happen if all of the linting issues have been resolved automatically. If there's issues that can't be automatically resolved, then the hook errors and doesn't re-stage/commit.

Can you please elaborate on what happens in the JS ecosystem? I'm not aware of it as I haven't worked that much in the ecosystem.

The JS ecosystem largely uses husky for hook generation (equivalent of python's pre-commit), with lint-staged as the linter/formatter. When you go to commit a file, husky runs its pre-commit hook which is typically configured to trigger lint-staged.

The difference to Python/Ruff is that when lint-staged runs, it applies any auto-fixable solutions and if there's not any issues that it can't automatically fix it will re-stage & commit the result for the developer.

This means the JS workflow for solely auto-fixable linting issues is "commit a file" -> "see it applied fixes and commited", whereas the Python workflow (with Ruff) is "commit a file" -> "see that linting failed" -> "run linting manually" -> "see it's passing (because everything got auto-fixed)" -> "re-stage files" -> "commit again". The Python workflow is far more verbose, imo unnecessarily. Hope this helps.