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

update common pre-commit configs #12516

Merged

Conversation

jzhang533
Copy link
Collaborator

update common pre-commit configs and commit the results of running pre-commit run -a

will mark this PR as WIP, since we may need to discuss are there any other checks needed for the repository.

please review .pre-commit-config.yaml file, all others are the results of running pre-commit run -a.

@jzhang533 jzhang533 requested review from GreatV and SWHL May 27, 2024 11:11
@@ -31,7 +27,7 @@ repos:
files: \.(c|cc|cxx|cpp|cu|h|hpp|hxx|cuh|proto)$
# For Python files
- repo: https://github.com/psf/black.git
Copy link
Collaborator

@SWHL SWHL May 27, 2024

Choose a reason for hiding this comment

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

I'm just curious why the Flake8 tool is used? Why not just use Black tools?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@SWHL These folders are excluded because they contain numerous bugs that are too time-consuming to fix and will be addressed at a later date.

hooks:
- id: check-added-large-files
args: ['--maxkb=512']
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm more concerned that 512kb won't be enough for our use, as some of the previous files had more than 512kb in them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will only be applied to local staged files as documented here: https://github.com/pre-commit/pre-commit-hooks?tab=readme-ov-file#check-added-large-files. which means, current .github/workflows/codestyle.yml will not capture large files being added.

Another option : increase the limit to 1024kb, and check all the files on each pull_request by using --enforce-all, at the same time, exclude existing large files. there are 28 files larger than 1M in the repo.

@@ -31,7 +27,7 @@ repos:
files: \.(c|cc|cxx|cpp|cu|h|hpp|hxx|cuh|proto)$
# For Python files
- repo: https://github.com/psf/black.git
Copy link
Collaborator

Choose a reason for hiding this comment

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

@SWHL These folders are excluded because they contain numerous bugs that are too time-consuming to fix and will be addressed at a later date.

@jzhang533 jzhang533 force-pushed the add_some_common_pre_commit_checks branch from cfcc492 to f722c18 Compare May 28, 2024 09:34
@jzhang533 jzhang533 force-pushed the add_some_common_pre_commit_checks branch from f722c18 to f05759a Compare May 29, 2024 03:36
@jzhang533 jzhang533 marked this pull request as ready for review May 29, 2024 03:50
Copy link
Collaborator

@GreatV GreatV left a comment

Choose a reason for hiding this comment

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

LGTM

@jzhang533 jzhang533 merged commit 24f06d1 into PaddlePaddle:main May 29, 2024
3 checks passed
@jzhang533 jzhang533 deleted the add_some_common_pre_commit_checks branch May 29, 2024 07:26
luzhongqiu pushed a commit to luzhongqiu/PaddleOCR that referenced this pull request Jun 3, 2024
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