Paste P425

Masterwork From Distant Lands
ActivePublic

Authored by davidedmundson on Jul 3 2019, 12:50 PM.
Clang format
==The problem==
I hate getting review comments on whitespace. It's very time consuming, it wastes reviewers 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 a computer should be doing.
Rather than going through reviews I would love to be able to just run one command and fix everything. This exists in a tool called clang-format. Other projects have 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 writing 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)
* add instructions on how to run as a local commit hook / arc lint.
Optionally:
* 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.
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 a badly formatted switch.
I don't think it's a helpful idea.
> "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.
> something something forced behaviour
Realistically not everyone follows frameworks style. It would need to be opt in at a per repo level.
> does it cover qml?
No :(
[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]
davidedmundson edited the content of this paste. (Show Details)Jul 3 2019, 12:50 PM
davidedmundson changed the title of this paste from untitled to Masterwork From Distant Lands.