Change Details

Clang format ==The problem== I hate review comments on whitespace. It's very time consuming, it wastes reviewers time, the committers time and most importantly it distracts from the actual content. I see it happen with new contributors, and I have absolutely no doubt we lose potential future developers at this step. It's a mundane task that a computer should be doing. Rather than going through reviews line-by-line I would love to be able to just run one command and fix everything. This exists in a tool called clang-format. Other projects use this already. I would like KDE to discuss having the same, I think it'll make reviews smoother, ease onboarding and free up valuable time for code. ==A practical roll out== We would need to do the following: * settle on a clang-format configuration * run across all the relevant KDE codebase (with some form of human review) Optionally: * Add to default arc lint file * have a git push hook on the server that rejects patches that fail the linter * have it as a step in the gitlab pipeline, though I don't think this is strictly necessary to do a roll out. Even without the optional steps it still becomes easier to compress all review comments about whitespace into a single comment. There's a great blog series [1] documenting MongoDB's port which is very thorough. Blender [2] interestingly doesn't seem to have any server side hooks or verification. ==Testing== At a recent sprint we ran [3] on plasma-workspace and reviewed the diff. There were a lot of changes, but nothing drastic. There's a slight issue with multi-line q_property declarations. At best i can set a flag to port them all to one line. Does someone have an idea on how to solve that? ==The inevitable questions== >"Can we check and format only the changed lines?" Technically yes. The command becomes quite a bit more complex. Pragmatically it's weird to tell someone to follow a style that isn't used elsewhere in that file. Especially when that diff only contains a move, or you add one entry into an existing badly formatted switch. I tried. The results were weird. > "it destroys git history!" Knowing how to quickly select a parent revision in your chosen git ui is an essential part of using git annotation anyway, and the argument that it destroys history makes no sense, but in case someone wonders. "git blame -w" acts as follows: - Re-ordering of includes does show up as a change - Other changes, including moving a brace to a new line, do not If you do want to skip the revision in git blame completely, one can also use git hyper-blame which can skip entire commits from a provided blacklist > something something forced behaviour Realistically not everyone follows frameworks style. It would need to be opt in at a per repo level. For the plasma-devel side of the mailing list, I would like to propose doing everything except: kwin, oxygen, breeze > does it cover qml? No. I am unaware of a tool which does. [1] https://engineering.mongodb.com/post/succeeding-with-clangformat-part-1-pitfalls-and-planning [2] https://wiki.blender.org/wiki/Tools/ClangFormat#Migrating_Branches_and_Patches [3] https://phabricator.kde.org/P434 place this in the top level of the source folder as .clang-format

Clang format ==The problem== I hate review comments on whitespace. It's very time consuming, it wastes reviewers time, the committers time and most importantly it distracts from the actual content. I see it happen with new contributors, and I have absolutely no doubt we lose potential future developers at this step. It's a mundane task that a computer should be doing. Rather than going through reviews line-by-line I would love to be able to just run one command and fix everything. This exists in a tool called clang-format. Other projects use this already. I would like KDE to discuss having the same, I think it'll make reviews smoother, ease onboarding and free up valuable time for code. ==A practical roll out== We would need to do the following: * settle on a clang-format configuration * run across all the relevant KDE codebase (with some form of human review) Optionally: * Add to default arc lint file * have a git push hook on the server that rejects patches that fail the linter * have it as a step in the gitlab pipeline, though I don't think this is strictly necessary to do a roll out. Even without the optional steps it still becomes easier to compress all review comments about whitespace into a single comment. There's a great blog series [1] documenting MongoDB's port which is very thorough. Blender [2] interestingly doesn't seem to have any server side hooks or verification. ==Testing== At a recent sprint we ran [3] on plasma-workspace and reviewed the diff. There were a lot of changes, but nothing drastic. There's a slight issue with multi-line q_property declarations. At best i can set a flag to port them all to one line. Does someone have an idea on how to solve that? ==The inevitable questions== >"Can we check and format only the changed lines?" Technically yes. The command becomes quite a bit more complex. Pragmatically it's weird to tell someone to follow a style that isn't used elsewhere in that file. Especially when that diff only contains a move, or you add one entry into an existing badly formatted switch. I tried. The results were weird. > "it destroys git history!" Knowing how to quickly select a parent revision in your chosen git ui is an essential part of using git annotation anyway, and the argument that it destroys history makes no sense, but in case someone wonders. "git blame -w" acts as follows: - Re-ordering of includes does show up as a change - Other changes, including moving a brace to a new line, do not If you do want to skip the revision in git blame completely, one can also use git hyper-blame which can skip entire commits from a provided blacklist > something something forced behaviour Realistically not everyone follows frameworks style. It would need to be opt in at a per repo level. For the plasma-devel side of the mailing list, I would like to propose doing everything except: oxygen, breeze, kwin(?) > does it cover qml? No. I am unaware of a tool which does. [1] https://engineering.mongodb.com/post/succeeding-with-clangformat-part-1-pitfalls-and-planning [2] https://wiki.blender.org/wiki/Tools/ClangFormat#Migrating_Branches_and_Patches [3] https://phabricator.kde.org/P434 place this in the top level of the source folder as .clang-format

Clang format ==The problem== I hate review comments on whitespace. It's very time consuming, it wastes reviewers time, the committers time and most importantly it distracts from the actual content. I see it happen with new contributors, and I have absolutely no doubt we lose potential future developers at this step. It's a mundane task that a computer should be doing. Rather than going through reviews line-by-line I would love to be able to just run one command and fix everything. This exists in a tool called clang-format. Other projects use this already. I would like KDE to discuss having the same, I think it'll make reviews smoother, ease onboarding and free up valuable time for code. ==A practical roll out== We would need to do the following: * settle on a clang-format configuration * run across all the relevant KDE codebase (with some form of human review) Optionally: * Add to default arc lint file * have a git push hook on the server that rejects patches that fail the linter * have it as a step in the gitlab pipeline, though I don't think this is strictly necessary to do a roll out. Even without the optional steps it still becomes easier to compress all review comments about whitespace into a single comment. There's a great blog series [1] documenting MongoDB's port which is very thorough. Blender [2] interestingly doesn't seem to have any server side hooks or verification. ==Testing== At a recent sprint we ran [3] on plasma-workspace and reviewed the diff. There were a lot of changes, but nothing drastic. There's a slight issue with multi-line q_property declarations. At best i can set a flag to port them all to one line. Does someone have an idea on how to solve that? ==The inevitable questions== >"Can we check and format only the changed lines?" Technically yes. The command becomes quite a bit more complex. Pragmatically it's weird to tell someone to follow a style that isn't used elsewhere in that file. Especially when that diff only contains a move, or you add one entry into an existing badly formatted switch. I tried. The results were weird. > "it destroys git history!" Knowing how to quickly select a parent revision in your chosen git ui is an essential part of using git annotation anyway, and the argument that it destroys history makes no sense, but in case someone wonders. "git blame -w" acts as follows: - Re-ordering of includes does show up as a change - Other changes, including moving a brace to a new line, do not If you do want to skip the revision in git blame completely, one can also use git hyper-blame which can skip entire commits from a provided blacklist > something something forced behaviour Realistically not everyone follows frameworks style. It would need to be opt in at a per repo level. For the plasma-devel side of the mailing list, I would like to propose doing everything except: kwinoxygen, oxygenbreeze, breezekwin(?) > does it cover qml? No. I am unaware of a tool which does. [1] https://engineering.mongodb.com/post/succeeding-with-clangformat-part-1-pitfalls-and-planning [2] https://wiki.blender.org/wiki/Tools/ClangFormat#Migrating_Branches_and_Patches [3] https://phabricator.kde.org/P434 place this in the top level of the source folder as .clang-format