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

style: apply clang-format #4039

Merged
merged 2 commits into from
Dec 6, 2023
Merged

style: apply clang-format #4039

merged 2 commits into from
Dec 6, 2023

Conversation

zephvr
Copy link
Contributor

@zephvr zephvr commented Dec 4, 2023

Describe your PR, what does it fix/add?

This commit apply clang-format styling to all files.

I also added a check in CI for clang-format, to prevent future commit from having clang-format styling error.

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

This commit changes quite a few lines of codes, it might break some pending PR.

I only added the check in one of the three existing build job. I can add it to the other two for more homogeneity between jobs, or create a dedicated job.

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

Apart from the point raised above, it is ready for merging

@vaxerski
Copy link
Member

vaxerski commented Dec 4, 2023

I'd rather not, the problem is that my PC for some reason has a wonky clang-format.

the enum name { occurrences are due to that.

@zephvr
Copy link
Contributor Author

zephvr commented Dec 5, 2023

Your PC not following the configuration is weird.

The enum name { occurrences is manage by the BraceWrapping parameter. For Hyprland it is set to false for AfterEnum as describe in the configuration :

Hyprland/.clang-format

Lines 60 to 61 in 37d7a8c

BraceWrapping:
AfterEnum: false

Can you check which parameter is set on your PC by running clang-format --dump-config | grep AfterEnum ?

There is multiple way to check and fix clang-format error, I personally use meson with this command : ninja -C build clang-format. How do you do it ? It might influence your result

@vaxerski
Copy link
Member

vaxerski commented Dec 5, 2023

image

How do you do it ? It might influence your result

VScodium extension

@zephvr
Copy link
Contributor Author

zephvr commented Dec 6, 2023

Console side

Ok, does running clang-format -i src/**/*{cpp,hpp} in the project folder fixes the formatting ?
If it is the case the issue definitely lies in VSCodium side.

VScode side

I guess your are using this extension : xaver.clang-format. I tried it on my instance, and with default settings it work as expected.

Maybe there is an extension parameter leading to your wonky formatting. Can your check if you have any modified extension settings on your VSCodium ? For that :

  1. In VSCodium, press F1
  2. Type Open settings UI, first result
  3. Search for @ext:xaver.clang-format @modified, it should show modified extension settings
  4. If you have settings showing try resetting those.

@vaxerski
Copy link
Member

vaxerski commented Dec 6, 2023

ok I meddled with the settings (wrong extension; I use C/C++ by M$) and setting em to these:
image

seems to have worked. My bad.

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@zephvr
Copy link
Contributor Author

zephvr commented Dec 6, 2023

Great that you found out how to make it work

I just rebased my branch on main, re-run clang-format and created a separe ci job.
Should be ready to merge :)

.github/workflows/ci.yaml Outdated Show resolved Hide resolved
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@zephvr
Copy link
Contributor Author

zephvr commented Dec 6, 2023

Done

Copy link
Member

@vaxerski vaxerski left a comment

Choose a reason for hiding this comment

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

cool, thanks!

@vaxerski vaxerski merged commit 4a42344 into hyprwm:main Dec 6, 2023
10 checks passed
@zephvr zephvr deleted the clang-format branch December 10, 2023 16:29
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.

2 participants