Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lint with pre-commit #94

Merged
merged 1 commit into from
Jan 2, 2024
Merged

Conversation

akx
Copy link
Contributor

@akx akx commented Dec 28, 2023

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 akx marked this pull request as ready for review December 28, 2023 14:08
@akx
Copy link
Contributor Author

akx commented Dec 28, 2023

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

@RunDevelopment
Copy link
Member

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
Copy link
Contributor Author

akx commented Dec 29, 2023

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
Copy link
Member

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
Copy link
Member

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.

- id: ruff
args:
- --fix
- --unsafe-fixes
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this as it was mentioned in CONTRIBUTING.md too.

@joeyballentine joeyballentine merged commit 4074134 into chaiNNer-org:main Jan 2, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants