-
Notifications
You must be signed in to change notification settings - Fork 24
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
Enable InsertBraces in clang-format #1851
Conversation
Options to update clang-formater by @Molter73 in this comment: #1849 (comment) |
So the lint workflow is now broken because this line: collector/.github/workflows/lint.yml Line 21 in 3f10453
Is trying to install packages using pip on a system that has python managed by the OS (this is actually a good thing, a lot of systems have been broken by pip installing packages over the actual system ones). Additionally, the I suggest we change the pre-commit/action line for the following block instead: - name: Run pre-commit
run: |
python3 -m venv .venv
source .venv/bin/activate
python3 -m pip install pre-commit
pre-commit run --all-files Not as convenient as a prebuilt action, but it's only 4 lines of bash, so 🤷🏻♂️. |
Let's try it. |
Well it sorta worked, looks like the clang-format step failed, maybe we need to rebase this PR so the changes you did to the braces are picked up? Also, could you add There's also a lint error I was not expecting here: collector/integration-tests/container/perf/scripts/init.sh Lines 143 to 146 in 3f10453
That block seems unreachable because the previous if block does |
Discussed offline and decided I'll be taking over the PR, @SimonBaeumer sorry your small change got so out of hand 😅 |
3a69ddc
to
3429431
Compare
Additionaly, the lint job is now run in the ubuntu-24.04 runner. This is needed to use a newer version of clang-format that supports the InsertBraces check.
This should make the environment used for running the lint job closer to what most of the team has as local dev environments.
7847221
to
cdc5e0b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Looks like konflux is gonna time out, but we don't really care about it, so I'm merging as is. |
Description
Enable expand brackets clang-format for better readability.
#1851