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

Pre-commit #448

Merged
merged 7 commits into from
Jun 23, 2023
Merged

Pre-commit #448

merged 7 commits into from
Jun 23, 2023

Conversation

vwxyzjn
Copy link
Contributor

@vwxyzjn vwxyzjn commented Jun 20, 2023

You can reproduce the existing pipeline with pre-commit run --all-files (note that pre-commit however only applies to git's staged and already committed files). I didn't fix the typo pointed out by codespell because it might create too many merge conflicts with existing PRs.

image

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 20, 2023

The documentation is not available anymore as the PR was closed or merged.

@lvwerra
Copy link
Member

lvwerra commented Jun 21, 2023

Can you elaborate how to use it in a workflow? Can you register this as a git hook or would you run it manually?

@vwxyzjn
Copy link
Contributor Author

vwxyzjn commented Jun 21, 2023

There are two workflows to use it

  1. A lot of people use pre-commit install to register it with git hooks so it runs automatically when you commit. Personally I don't like it because it doesn't let you confirm your changes
  2. I like to use pre-commit run --all-files as a drop-in replacement for make format. The only difference is currently we manually specify check_dirs, whereas pre-commit's check_dirs will include all the committed and staged files (and don't include the untracked files).

@lvwerra
Copy link
Member

lvwerra commented Jun 23, 2023

Sounds good to me. What to you think about adding pre-commit run --all-files to the Makefile e.g. as make format. A few commands less to remember :)

What do you think @younesbelkada ?

@younesbelkada
Copy link
Contributor

the changes sounds good, I also agree we should add something in the lines that @lvwerra mentioned on the Makefile

@vwxyzjn
Copy link
Contributor Author

vwxyzjn commented Jun 23, 2023

@lvwerra @younesbelkada good idea! Done with make commit

Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@younesbelkada younesbelkada left a comment

Choose a reason for hiding this comment

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

Thank you @vwxyzjn !

@vwxyzjn vwxyzjn merged commit 82c8f20 into main Jun 23, 2023
@vwxyzjn vwxyzjn deleted the pre-commit branch June 23, 2023 15:37
@lvwerra lvwerra mentioned this pull request Jul 5, 2023
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.

4 participants