Lint with pre-commit - Githubissues

chaiNNer-org / spandrel

Spandrel gives your project support for various PyTorch architectures meant for AI Super-Resolution, restoration, and inpainting. Based on the model support implemented in chaiNNer.

MIT License

112 stars 7 forks source link

Lint with pre-commit #94

Closed akx closed 6 months ago

akx commented 6 months ago

This PR proposes to

add a .pre-commit-config.yaml file to make it possible to use https://pre-commit.com/ to run the Ruff lint and format step (i.e. have everything checked and fixed at commit time (if so desired)).
for consistency's sake, switch CI to do that too using pre-commit/action.
- on that note, it sets RUFF_OUTPUT_FORMAT to "github", so Ruff outputs warnings in a form that makes GitHub Actions print them quite prettily
~~add the basic pre-commit-hooks hooks to enforce some hygienic line-ending rules (and fixes the handful of issues).~~

This shouldn't affect regular development without pre-commit other than having to remember to keep the Ruff version in sync between the lint extra and the pre-commit config.

akx commented 6 months ago

If this doesn't feel like a chaiNNer or Spandrel thing to do, that's fine too!

RunDevelopment commented 6 months ago

I personally don't like pre commit hooks, so I appreciate it that this one is optional.

The main issue I see with it, is that it's not really optional. You can just choose when to run it (before commit, after push (by CI)) and still have to follow all of its rules. This can actually make not using the pre commit hook less user-friendly since some of the options you enabled (e.g. end-of-file-fixer) seem to be designed for automated fixing and would just be annoying to deal with manually. I can just paste a ruff command into my terminal to fix formatting, but there's nothing for those extra pre commit rules.

So I'm not sure about this.

akx commented 6 months ago

I personally don't like slow pre-commit hooks either (and don't get me started on slow terminal prompts, ugh), but these are very fast (and this is running everything on all files; in a commit hook, it'd only run on touched files):

$ hyperfine 'pre-commit run --all-files'
Benchmark 1: pre-commit run --all-files
  Time (mean ± σ):     413.4 ms ± 178.1 ms    [User: 847.4 ms, System: 391.5 ms]
  Range (min … max):   340.1 ms … 918.2 ms    10 runs

That said, I can absolutely get rid of the extra rules and just keep Ruff in :)

RunDevelopment commented 6 months ago

I personally don't like slow pre-commit hooks either

It's not just slow ones. I don't want my commits to fail, well, committing. I'd rather use IDE save actions to see what exactly is committed before committing.

That said, I can absolutely get rid of the extra rules and just keep Ruff in :)

Please do that. We can still re-add them later if need be.

RunDevelopment commented 6 months ago

Alright, with my mistake resolved, I think this is basically ready. Could you please resolve the conflict?

Also, we should probably document this in CONTRIBUTING.md.