sailfishos / droidmedia

15 stars 58 forks source link

Clang format #126

Open simonschmeisser opened 7 months ago

simonschmeisser commented 7 months ago

I experimented a bit with adding clang-format to this repo. The code is unfortunately formated quite creatively so I was not able to decide what is likely the desired formatting. Please feel invited to comment as much as you like and I'll try to improve the settings.

Or do you have an internal clang format file already?

I choose clang-format-12 as that should be available on both Ubuntu 20.04 and 22.04 but if that is not what you use at Jolla I can also propose another version.

I added pre-commit.com which can be configured to run before any commit conveniently. This way your commits will always be clean.

I then formatted the code with some example settings and added the commit hash to .git-blame-ignore-revs. Some git blame tools understand this and won't show the formatting in blames.

Finally I added github ci to run this online in case contributors do not locally.

sudo apt install clang-format-12
pip install pre-commit
pre-commit install

run for all files with

pre-commit run -a

or just have it complain & format once you git commit

simonschmeisser commented 7 months ago

Tools like clang format work best if you run them automatically on each commit. Then you never again have to worry or discuss about code formatting and can concentrate on the content. As long as you stick to a certain clang-format version the results will be stable, ie no unrelated diff in any future diff even though you run this on each and every file on each commit.

You mean to check if the rules where applied? Hooks can be run on the developer side to check if code follows the rules a shown e.g. in: https://invent.kde.org/frameworks/extra-cmake-modules/-/blob/master/kde-modules/kde-git-commit-hooks/clang-format.sh

I picked clang-format-12 here as it is available on Ubuntu 20.04 and 22.04 but if you say people at jolla use 22.04 or similar newish distros clang-format-14 would also be an option

I think Clang 12 is to old if it does limit us. The current stable Clang version is 17, 12 is to far away I think.

simonschmeisser commented 7 months ago

I found there is actually a clang-format file for Qt with BSD license and applied it. Need to clean up the history ...

Thaodan commented 7 months ago

I found there is actually a clang-format file for Qt with BSD license and applied it. Need to clean up the history ...

The KDE Clangformat file has been quite useful. It also follows the Qt coding style almost exactly. This project doesn't use cmake but for any that does the ecm can add hooks when running cmake.

In my project below I'm using those hooks. They tell me to format the code before commiting. It's as easy running ninja/make -C build clang-format

Thaodan commented 7 months ago

What I'm not sure of is having hooks on clang as in my experience that sort of tooling might be too easily complaining on code details that are perfectly fine, like forcing specific line wrapping etc. Elsewhere we've cleaned up messes by running a formatting tool just once and after that people honoring the conventions better. We could start by just doing the same thing here.

Clang-format configuration can be adjusted per project or optionally per project type e.g. Qt projects.

As the formatting in specific sections can be disable hooks should be always on, unless the commiter tells git to not do so, without it easy for new contributors to miss following the formatting rules.

You add disable formatting for specific section by inserting:

// clang-format off
Some fragile or from third parties imported code...
// clang-format on

as shown below: https://community.kde.org/Policies/Frameworks_Coding_Style#Disabling_formatting_for_specific_parts