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 hook to use clang-format #3

Merged
merged 10 commits into from
Feb 5, 2023

Conversation

miguelteixeiraa
Copy link
Contributor

@miguelteixeiraa miguelteixeiraa commented Feb 5, 2023

Hello! I am proposing to use clang-format as a pre-commit hook.
For this I used the pre-commit micro framework, but I can remove it and use simple 'cp' commands if you think would be better. I also added usage details in the readme.
For this clang-format configuration I used the BasedOnStyle: Google, but it was just to initialize, we can discuss here which style base is better or just create a custom one. There is also a clang-format config detector that analyses the code and create the configurations but it is a windows-based software and I'm not able to try it now.

@lemire
Copy link
Member

lemire commented Feb 5, 2023

Let us try to merge this and see what happens.

@lemire lemire merged commit 88d341d into ada-url:main Feb 5, 2023
@lemire
Copy link
Member

lemire commented Feb 5, 2023

merged

@anonrig
Copy link
Member

anonrig commented Feb 5, 2023

I'm -1 on having pre-commit hooks, since it moves the requirement of running the checks to the developer. Right now, there isn't any way for us to verify that the pull request sent to ada/idna is compliant with our clang-format. We should have a github workflow for validation purposes.

@miguelteixeiraa
Copy link
Contributor Author

I like pre-commit hooks because it kinda ensure that things happened before I commit. But yeah, it is easily bypassed or just ignored.
@anonrig So do you think pre-commit should be removed + add a workflow to check the code style, or just add a workflow to check the code style?

@anonrig
Copy link
Member

anonrig commented Feb 5, 2023

I think pre-commit should be a recommendation to reduce CI errors, but we should definitely have a GitHub workflow hook if we want to make sure we are following the formatting guidelines.

@miguelteixeiraa
Copy link
Contributor Author

Sounds good!

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.

None yet

3 participants