evals icon indicating copy to clipboard operation
evals copied to clipboard

Suggestion: add git-hook hook for pre-commit

Open machinekoder opened this issue 1 year ago • 1 comments

Since a run of pre-commit is required before creating a PR, it would be reasonable to add pre-commit as git-hook. We use this in our development repos and have very good experience since it ensures everyone is actually running pre-commit.

At the moment, it looks like only part of the repo is actually conveying the code standards enforced by the pre-commit tasks, as can be verified by running pre-commit run --all-files. Running pre-commit in the CI might help-as well.

If you think that sounds reasonable, I'd be happy to create a PR.

machinekoder avatar Mar 21 '23 13:03 machinekoder

I think pre-commit install will install it as an actual git-hook, but I see your point that it is not committed into the repo by default.

There may be some intention or concern to not do so because the comments mention that it is slightly dangerous:

https://github.com/openai/evals/blob/f118fca38e3dde127d2eba44374806f581b0da1c/.pre-commit-config.yaml#L16-L29

A manual run adds a kind of self-check to confirm the existence of unused-in-file imports. I see this visual check as a good thing in library imports like __init__.py (which is excluded in the code above) where the intention is to expose functionality but not necessarily use them within the file.

jonathanagustin avatar Mar 21 '23 14:03 jonathanagustin