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

ci(clang-format): directly do the clang-format instead of error #8955

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

littleblack111
Copy link
Contributor

Describe your PR, what does it fix/add?

as title suggestes, atm clang-format will just give error and we have to manually fix it, this PR makes it so it automatically does it for us

Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)

if you dont mind the github actions bot spamming commits and growing in the contribution graph ig

Is it ready for merging, or does it need work?

tested in another repo

@littleblack111
Copy link
Contributor Author

Lmk if u prefer executing the push via the command instead

@vaxerski
Copy link
Member

vaxerski commented Jan 4, 2025

I really don't like this. I don't like bots committing to the repo in general.

@littleblack111
Copy link
Contributor Author

Eh ok :(

@littleblack111
Copy link
Contributor Author

How about bot suggesting commits via review?

@littleblack111 littleblack111 reopened this Jan 5, 2025
@littleblack111
Copy link
Contributor Author

littleblack111 commented Jan 5, 2025

How about bot suggesting commits via review?

eh? @vaxerski

@vaxerski
Copy link
Member

vaxerski commented Jan 5, 2025

that sounds fine, depending on how it's done. If it just attaches a patch that's okay, as long as its not spammy.

@littleblack111
Copy link
Contributor Author

See the review.

Then you'd have an option to just press commit

Then one of them will be author and another will be co author of that commit

@vaxerski
Copy link
Member

vaxerski commented Jan 6, 2025

what if there are 20 files needing 10 patches each? Massive spam.

@littleblack111
Copy link
Contributor Author

alright, preview: littleblack111#1 (comment)

@vaxerski
Copy link
Member

vaxerski commented Jan 8, 2025

yep, that looks good. Two things, firstly, the message needs to be nicer and more bot-like:

Please fix the formatting issues by running `clang-format`, or directly apply this patch:

Then, put the patch in a spoiler

like this
boo

@littleblack111
Copy link
Contributor Author

alright

@vaxerski
Copy link
Member

vaxerski commented Jan 8, 2025

thanks! Much better.

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