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

Add clang-format #110

Conversation

DeveloperPaul123
Copy link
Contributor

@DeveloperPaul123 DeveloperPaul123 commented Aug 1, 2023

This clang-format was created using LLVM's tool to "detect" the format from existing code. From my testing, it's quite close.

@tcbrindle
Copy link
Owner

Hi @DeveloperPaul123!

Please could you submit these changes as two separate PRs? The .gitignore update looks good and I'll commit it right away, but I want to try out the clang-format file first before deciding whether to add it (I've found that clang-format destroys my hand-crafted nice formatting as often as it fixes things...)

@DeveloperPaul123
Copy link
Contributor Author

@tcbrindle Seems reasonable! Done in #111

@tcbrindle
Copy link
Owner

@tcbrindle Seems reasonable! Done in #111

...and merged!

This clang format file was generated using LLVMs tool to "detect" code style from existing code files. It's pretty close to what was already there.
@DeveloperPaul123 DeveloperPaul123 changed the title Add clang-format and update gitignore Add clang-format Aug 1, 2023
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.67%. Comparing base (983f1cb) to head (c3d4a15).
Report is 335 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #110   +/-   ##
=======================================
  Coverage   97.67%   97.67%           
=======================================
  Files          66       66           
  Lines        2236     2236           
=======================================
  Hits         2184     2184           
  Misses         52       52           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DeveloperPaul123
Copy link
Contributor Author

@tcbrindle Any comments on the clang-format? Or any formatting changes you'd like to see in the config file?

@tcbrindle
Copy link
Owner

I do like the idea of having a project-wide clang format config to keep everything consistent, and to make life easier for new contributors.

The only problem is that I'm very fussy when it comes to how I like my code formatted!

I still need to have a play with all the clang-format options until I find the combination I like.

@DeveloperPaul123
Copy link
Contributor Author

I completely understand. If you have examples of styling that you want to keep that clang-format is breaking, feel free to post them here and I can try to update the clang format file.

@Guekka
Copy link
Contributor

Guekka commented Jul 19, 2024

While contributing on #192, at least half of the reviews were about formatting. This creates work for both the contributors and the maintainer(s) that could be automated. I understand your desire of keeping your code formatted exactly as you like, but I think this is a case where consistency and automation beat preference.
I'm willing to update this PR if needed. Would you consider merging it @tcbrindle?

@DeveloperPaul123
Copy link
Contributor Author

I honestly forgot this was still open. I'm also happy to address any concerns to get this merged in.

tcbrindle added a commit that referenced this pull request Aug 15, 2024
After a lot of messing about, I've think I've finally come up with a clang-format config that I don't hate. It still messes up the concept definitions in `core/concepts.hpp`, but other that that it seems to do a pretty good job of matching the current layout.

I'm not planning on reformatting the whole codebase or anything like that but this should hopefully be useful for new contributors.

Supercedes #110
@tcbrindle tcbrindle mentioned this pull request Aug 15, 2024
@tcbrindle
Copy link
Owner

I've finally added a .clang-format config for the project in PR #199. Thanks @DeveloperPaul123 for the initial work on this and @Guekka for reminding me that I should actually do it :)

@tcbrindle tcbrindle closed this Aug 15, 2024
@DeveloperPaul123
Copy link
Contributor Author

No problem 👍

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