Skip to content

Pre-commit#448

Merged
vwxyzjn merged 7 commits intomainfrom
pre-commit
Jun 23, 2023
Merged

Pre-commit#448
vwxyzjn merged 7 commits intomainfrom
pre-commit

Conversation

@vwxyzjn
Copy link
Copy Markdown
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
Copy Markdown

HuggingFaceDocBuilderDev commented Jun 20, 2023

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

@lvwerra
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@vwxyzjn
Copy link
Copy Markdown
Contributor Author

vwxyzjn commented Jun 23, 2023

@lvwerra @younesbelkada good idea! Done with make commit

Comment thread Makefile Outdated
Copy link
Copy Markdown
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
yxliu-TAMU pushed a commit to mincheolseong/ECEN743-GRPO-Project-Proposal that referenced this pull request Apr 20, 2025
* Pre-commit

* modify CI

* modify make file

* temporarily disable codespell

* update make file

* update contribution guide

* pushc changes
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