Add .clang-format file and format the src/ and include/ with it.

[BumblingCoder]

The .clang-format file attempts to follow the style described on the wiki and existing in the code. Having this file will make it easier for contributors to keep a consistent style within the application.

[tresf]

@BumblingCoder thanks for this. The project is in dire need of code consistency.

Since stable-1.2 is bug-fix only, we'd cannot accept this against the stable-1.2 branch. This needs to be based on master to be accepted, sorry for the inconvenience this will cause.

A few more notes...

1. We won't be mandating spaces inside parenthesis. e.g. AboutDialog( QWidget * parent = 0 ); should remain without spaces.
2. Some of the style guidelines haven't been decided. e.g. Type* name versus Type * name versus Type *name. We should iron these out on discord #devtalk.
3. We need to automate this. For starters, simply providing a mechanism to error on Travis is sufficient. We're already doing this for the shellcheck utility in a Travis-CI target called style. Automating this isn't mandatory in your PR but it is preferred.
4. Out of curiosity are there any alternatives to this tool that are just as good? Is this file recognized by non-Clang compilers? Does the file need to reside in the base of our source tree? Any background on this is appreciated even if it seems obvious.

Thanks again.

[BumblingCoder]

For 1 and 2 I tried to match the existing style of the code as much as possible. These seemed to be the most common style options in the existing code (though that isn't very important for a reformat.)

Redoing this on master should be easy, the only changes not made by script were the clang-format files and moving as single header that has to not be in alphabetical order.
I can discuss the options to use in discord and create a new PR based on master.

3. I could add a ci stage that checks for formatting errors, that is pretty easy.

4. There are other tools, but my understanding is that none are as good as clang-format. Clang-format is a standalone tool built on top of clang, but you can build with any compiler and use it. When running clang-format for a file it looks first in the directory with the file and then in each directory above that to find .clang-format file. This allows you to have different format files for different parts of the code, which I did for the plugins directory as it doesn't make sense to enforce a style for third-party code.

[tresf]

it doesn't make sense to enforce a style for third-party code.

Agreed however many of the plugins are not third-party code, we just don't organize it well yet. We'll need to provide you with that information.

[BumblingCoder]

I created an issue to discuss the specifics of what a clang-format file should be. #3856

[lukas-w]

Qt Creator appears to support .clang-format, so I guess it's not a bad candidate: http://doc.qt.io/qtcreator/creator-beautifier.html

We need to automate this. For starters, simply providing a mechanism to error on Travis is sufficient.

Please no. It's bad enough the way it is, having your Travis build fail because of pedantic warnings, fixing these by trial-and-error because your compiler's warnings differ from the ones on Travis. It'll be extra annoying having your PR fail because of single "wrongly"-placed spaces, e.g. you wrote if( instead of if ( or writing return;  instead of return; . All PRs should be reviewed before being merged anyway, if formatting is off too much the reviewer can point it out.
Making formatting an automated task will clutter the commit history, making it a lot harder to inspect. Which would be unfortunate as with the original authors having left, commit history is one of the few ways left to learn about the codebase.
What we need are proper unit tests on Travis so we can see when a PR breaks a feature, not when a PR breaks code formatting by a single character. Everything else just distracts from what's important and hinders development.

[BumblingCoder]

If you enforce the code style it actually has the effect of less formatting changes in the future. The idea is that the entire code base gets formatted in one big commit and then you don't see formatting changes in other random commits. It the style is not enforced you well see additional changes in the history when short edits a file and tigers formatting of the whole thing, changing code unrelated to their work because somebody else committed code that didn't match the style. Having the format enforced also means that the reviewer doesn't have to spend any time worrying about formatting because the automatic ci systems will tell them if it is wrong.

As for formatting being a burden: it is easy to integrate clang-format into basically every editor, there are scripts that will run it on your locally changed files, and it is easy to set up as a client site git hook. There is no reason for the developer to manually worry about formatting, just let clang-format handle it.

[tresf]

What we need are proper unit tests on Travis so we can see when a PR breaks a feature, not when a PR breaks code formatting by a single character. Everything else just distracts from what's important and hinders development.

We already have a style task and it works well for fixing shellcheck issues. I'm not sure how this is different. It avoids the cat-and-mouse game that has been occurring when code is otherwise good to merge. It makes it so that PRs will rarely ask "Can you please reformat..." type questions.

Having the format enforced also means that the reviewer doesn't have to spend any time worrying about formatting because the automatic ci systems will tell them if it is wrong.

Agreed.

Qt Creator appears to support .clang-format, so I guess it's not a bad candidate: http://doc.qt.io/qtcreator/creator-beautifier.html

So the Travis-CI check becomes moot as we standardize on an IDE, no?

[tresf]

It'll be extra annoying having your PR fail because of single "wrongly"-placed spaces, e.g. you wrote if( instead of if ( or writing return; instead of return;.

It will only fail on the style check. It's no more or less annoying than it failing on MacOS for NO REASON AT ALL. :)

Making formatting an automated task will clutter the commit history, making it a lot harder to inspect.

I tend to agree with this. If Travis-CI is catching the style issues, we can just do it as we go, although we already have automatic commit bots for translations and we'll be doing the same thing with the authors/CONTRIBUTORS file soon, so the cluttering of the commit history is something we can mitigate but we cannot avoid.

[lukas-w]

The idea is that the entire code base gets formatted in one big commit and then you don't see formatting changes in other random commits

I'm all for that one big commit. 👍

We already have a style task and it works well for fixing shellcheck issues. I'm not sure how this is different.

Shellcheck checks for potential bugs though, not formatting, right? So it's more similar to the -Werror flag, which I'm not very fond of either. I never liked -Werror or shellcheck to begin with, and this idea is taking it one step further.

It avoids the cat-and-mouse game that has been occurring when code is otherwise good to merge

You'll just be playing the same game against the CI instead, searching through Travis logs.

So the Travis-CI check becomes moot as we standardize on an IDE, no?

In theory, sure, if we can get all contributors to use the same IDE, and if there are no version differences in clang-format that cause different formatting.

I think it may alienate new contributors. Personally, I'd feel discouraged to contribute to a project that tries to enforce an IDE onto me and has strict CI checks that enforce every space in the code be at the place a program determines to be right. This level of nit-picking in quality control appears ridiculous to me for a project with such a broken architecture and no unit tests whatsoever.

[tresf]

Shellcheck checks for potential bugs though, not formatting, right? So it's more similar to the -Werror flag, which I'm not very fond of either. I never liked -Werror or shellcheck to begin with, and this idea is taking it one step further.

Well, the -Werror is bad because it prevents otherwise good builds from succeeding and often the warnings are out of our control due to an upstream issue. We've also seen evidence it's probably more work than it's worth. Shellcheck on the other hand is completely under our control. So is style. Perhaps a second Travis-CI checkmark is a better way to handle the style controls so that otherwise good code doesn't have the appearance of "All checks failed" on the surface of a PR.

You'll just be playing the same game against the CI instead, searching through Travis logs.

Arguably, someone will, but it would be the code submitter (no longer a project admin). The style target had this very "policing" in mind. I'm all for making it nicer to the code submitter, but I think it will reduce the amount of style chatter. To quote @zapashcanon:

also [#3476 aims] to ease the process of adding a "style build" which has nothing in common with other builds.

...

I'm all for that one big commit. 👍

👍

Personally, I'd feel discouraged to contribute to a project that tries to enforce an IDE onto me and has strict CI checks that enforce every space in the code be at the place a program determines to be right.

Yeah, perhaps this idea is too wishful thinking and not based in reality. Many submitters just want to patch a problem, they don't want to download configure and fire up an IDE. That said, if we get major contributors that help refactor, redesign and enhance the product more than a few source files, standardizing the IDE is really just a step in taking on new talent. But it's certainly not coupled to this PR.

This level of nit-picking in quality control appears ridiculous to me for a project with such a broken architecture and no unit tests whatsoever.

I'm not sure how I feel about this statement. I've submitted PRs to some projects that don't police this at all -- have a mess of formatting, commenting, indenting. I've seen other projects that are extremely strict and even police the way the PRs are titled. I don't think a style check is unreasonable/ridiculous although I do think that it can have the appearance of unreasonable given our current CI verbiage.

[lukas-w]

Reopening so we don't lose track of it. I think this should be re-done for master once 1.2 is released.

[redianthus]

I think it may alienate new contributors. Personally, I'd feel discouraged to contribute to a project that tries to enforce an IDE onto me [...]

A few things to avoid that: editor config, using a .gitattributes.

[JohannesLorenz]

For this issue, I see no files changed.

I'm sorry that I didn't see this PR when opening #4690 . I think we should close this one now in favor of #4690 . If no vetos, I'll close in 7 days.

@zapashcanon An editorconfig has been added in the last weeks. .gitattributes already exists, but feel free to open an issue/a PR for adding things.

[zonkmachine]

For this issue, I see no files changed.

I'm sorry that I didn't see this PR when opening #4690 . I think we should close this one now in favor of #4690 . If no vetos, I'll close in 7 days.

I also see no changes in the two commits. Closing.
@BumblingCoder Thanks!