fix and enforce convention on white-spaces, code-indentation and other coding style

[KrisThielemans]

Current code uses a mixture of conventions from different editors etc. We've had some discussion on this on the stir-devel mailing list, see https://sourceforge.net/p/stir/mailman/message/34996792/.

There's a few issues here:

1. how will git handles changes in white-space (as I'm worried about generating tons of conflicts in future branch/PR merges)
2. what style and tool to use
3. how to enforce that future commits use the correct style

Following comments address these.

[KrisThielemans]

@NikEfth has done some more experiments on issue 1 above (how git handles changes in white-space). Here's his email, essentially saying that as long as we make sure that white-spaces get corrected before any merge, we should be fine.

I did the small git experiment:
I used the astyle application because it has great reviews and interfaces to 3-4 major IDEs.
In addition, it can be setup from an external text file, which we can supply and tell everybody just
to use this file when they call the application.

In the style I made, inside functions I delete the empty lines, unless there is some sort of block around,
were I create new empty lines to make the block stand out.

Step 1:
Format Scanner [.cxx and .h] in a specific style using the astyle application in branch "style2" (history).
commit and pushed

Step 2:
Format Scanner [cxx and h] with the same style in master.

Step 3:
pull request
The diff command in git hub interface continued to show differences between the files. Even that they
were identical. But no conflicts emerged.

Step 4:
In branch "style 1" which has a modified Scanner.cxx with different options and is older from "style 2" I tried to merge the new format from master using:

git merge origin/master

and I had conflicts in the Scanner.cxx.

Step 5:
In style 1 I formated the Scanner.cxx as it is in the master and the merge returned: Already up-to-date.

----------- Experiment 2
Now I will repeat the same but in addition to the formatting I will add and remove some code.

Step 1:
New branch "code 1"

Step 2:
Format with astyle only the master.

Step 3:
From arccorrection.cxx I delete a function and add a new one.

Step 4:
Merge the formated code form master to the code1 : Failed.

Format the file in code1

Merge : Success

[KrisThielemans]

Issue 2: tool/conventions to use

I believe that a lot of STIR was formatted with emacs default c++-mode settings (no tabs), but this might have become a minor fraction by now.

Nikos used astyle with the following

--style=allman
--indent=spaces=2

--attach-extern-c

--indent-classes
--indent-modifiers
--indent-switches
--indent-cases
--indent-namespaces
--indent-labels
# This could be removed, as well 
--indent-preproc-block
--indent-preproc-define
# Not in Style2 
#--indent-preproc-cond
--indent-col1-comments

--break-blocks=all
--pad-oper
--pad-header

--unpad-paren

--align-pointer=type
--align-reference=type
--delete-empty-lines

Another well-known tool is clang-format. This might not do matlab or Python (not sure).

[KrisThielemans]

Issue 3: git integration

we'd have to prevent new commits messing things up again, while not making a contributor's life too complicated. There's a lot of tools for this, involving git hooks, server-side stuff etc. Advice sorely needed. Possibly @dvolgyes, @casperdcl can help?

Examples used by CMake are at https://gitlab.kitware.com/cmake/cmake. This combines usage of a .clang-format, modification of .git-attributes and some hooks.
I've noticed that with this a "git diff" automatically colors white-space errors (but the hooks don't get set automatically).
There's also a git plugin for helping with clang-format integration, see for instance https://dx13.co.uk/articles/2015/4/3/Setting-up-git-clang-format.html.

[casperdcl]

I always have a .clang-format, but to avoid contributors from having to have it installed (and also git saving/overwriting files while they're open and modified in an IDE/text editor) I avoid adding a hook. It's up to devs to ensure formatting is correct before a merge.

[casperdcl]

To be explicit, I'd propose:

• (repo) .gitattributes

[attr]cppfiles  eol=lf  filter=cppfmt
[attr]cfiles    eol=lf  filter=cppfmt
[attr]pyfiles   eol=lf  filter=pyfmt
*.cpp  cppfiles
*.hpp  cppfiles
*.h    cfiles
*.c    cfiles
*.py   pyfiles

• (repo) .clang-format

---
Language:      Cpp
BasedOnStyle:  LLVM
Standard:      Cpp11
...

• (repo) .style.yapf

[style]
split_before_named_assigns=False

• (local) config

$ git config --global filter.cppfmt.clean clang-format
$ git config --global filter.pyfmt.clean yapf
$ sudo apt-get install clang-format
$ pip install yapf

Not sure about linting for matlab files.

[casperdcl]

The other advantage of the above is users can easily turn off linting (e.g. if it takes too long):

~/UCL/STIR$ git config --local filter.cppfmt.clean cat

And of course if filter.cppfmt is undefined (never configured because you're a casual end user) then the filter is silently ignored.

[dvolgyes]

Well, there are a lot of technical issues here.

First, there is no way to actually prevent a git commit. Period.
It happens on the user's side, he/she can do anything.
(But you can help the users to follow the guides.)

This is the reason why github has no pre-commit hook.
The commit has already happened on the users computer.
If he/she has rights to update the repository, there is no way to prevent this.

However, you can enforce rules through the pull requests:
you don't have to merge, if you are not satisfied with the PR.

There are several bots, like the "pep8speaks" which tests the PR,
( https://pep8speaks.com/ )
and comments it. I am not aware any C++ version, but it might exist.
You can take a look at Github's market place and the "works-with" directory.
https://github.com/marketplace
https://github.com/works-with

The usual way to enforce special rules, like style, is through continuous integration.

• make code formatting (lint, static analyzer, etc.) part of the tests
• if the PR does not conform with the rules, then the build process will fail
• Travis/AppVeyor can upload artifacts to a site, so if the log isn't detailed enough, then you
  could check this file for further details

This has the advantage that any tool can be used, even quite specialized one.
For instance, the documentation could be spell-checked, and if the spell-check fails,
then the build fails. (But I wouldn't go that far.)

So what could be incorporated?

• astyle http://astyle.sourceforge.net/
• uncrustify http://uncrustify.sourceforge.net/

I liked astyle more, but uncrustify seems to be more maintained.
(It has Travis builds, recent commits, etc.)

Theoretically, anything could work.
The rules could be enforced with local pre-commit hooks,
or with a simple make file, like "make format" before commit.

But I would argue for a server-side protection. And the only way I see this
feasible, is through pull requests. If there is a "pep8speaks"-like c++ bot,
then it could work. Otherwise I would argue for a
"make test-format" option, where astyle/etc. could format the code,
git detect if there is a modification (git status), and if there was one,
then the code did not pass the style check.

Astyle/uncrustify or anything else: there are dozens of similar apps, starting with clang tools,
but, I haven't seen any integrated one for github.

In the end, it depends on the usability, etc. Enforcing clang code formatter
on windows or Mac could be quite challenging. And the same for installing a working pip version.
So I would not modify the git config files.
I think the initial cost contributing to STIR is already high enough.

Enforcing through Travis is easier, but it has the disadvantage that the actual debugging (figuring out what is wrong with the commit) could be more challenging. (It might be hard to find in the log file.)

That's my opinion. (Besides that: I do not really care. I try to keep the current code style of the file, and if I just want to understand the code, then I enforce my own rules, make a patch, and reformat the patch in the old style.)

astyle/emacs/vi/eclipse/etc.: It would be nice to choose a universally applicable style but it is not always possible. E.g. tools differ in tiny things, like: do they count the line length from the first character or the first non-whitespace character? (The answer depends on the tool.)

[casperdcl]

@dvolgyes I think @KrisThielemans's point was that we want liniting but don't want the casual user to be put off because of it. So my suggestion was to:

1. Add linting config files to the repo (.gitattributes, .clang-format, .style.yapf)
2. Tell maintainers to configure local hooks and install linters
3. Protect master branch so only said maintainers can push to it
4. As you suggest add checks to .travis.yml (where it's easy to apt-get or pip install things, and parse clang-format --output-replacements-xml or python -m flake8 --exit-zero without doing any inplace-lint-and-check-git-status-for-modifications)

[KrisThielemans]

I have had another read at this. So this looks mostly ok to me.

there's 2 things I'd like to get some feedback on:

1. as we're not imposing this on the (casual) contributor, how do we handle PRs? I guess, the contributor creates the PR, we run the tool to clean it all up, we commit that to the PR, and tell the contributor to check.

2. we need guidelines for (not so casual) contributors on editor settings. this might be hard, but we should cover the most important ones. I guess this point will determine the final style/tool that we use.

Anyone wants to create a PR for this?

[dvolgyes]

I have no useful idea for cleaning up PRs, but there is one for the enforcement part.
There is a tool called precommit-hooks. (See pre-commit.com and https://github.com/pre-commit/pre-commit-hooks ).
This is a framework for handling git pre-commit hooks, and it is extendable with plugins which are easy to write.
The configuration file is stored in the .pre-commit-config.yaml, and it is ignored, unless the developer actively enables it with:
"pre-commit install"
After that every commit is checked with the hooks, but they can be disabled for false positives.
The hook even can re-write the code, e.g. with astyle. This aborts the commit because it is a bad idea to check in code without taking a look, but with repeated attempt it will accept. (because second time the code does not change).

I was experimenting with the framework, and it is quite OK, e.g. I could write a plugin in 30 minutes which checks image/video/audio file integrity, and rejects corrupted files.

First of all, writing plugins is easy, secondly, it does not do anything, unless enabled.

It is pretty well designed, multi-platform, and I did not find anything at least similarly capable alternatives.
(Keep in mind, this is a meta-problem, you still have to decide about code style, etc.,
but this tool could enforce the decisions.)

[dvolgyes]

By the way, here is a longer list of hooks. I did not find astyle hook, but it shouldn't be too complicated.
https://pre-commit.com/hooks.html

[ashgillman]

Perhaps a format could be decided upon for new additions to begin with, with no formal enforcement (avoiding discussions on existing files, merges/PRs/forks, enforcement).

So far suggested have been astyle, clang-format and uncrustify. flake8 is pretty universal for Python (if you're interested, look at black, but it is too much for here).

[KrisThielemans]

EditorConfig seems pretty good...

[ashgillman]

EditorConfig doesn't have quite enough options.

E.g.,

function(a_long_arg,
         b_long_arg;
function(
  a_long_arg, b_long_arg);

curly_brackets {
  like_this;
}
curly_brackets
{
  like_that;
}

EDIT: yet https://github.com/editorconfig/editorconfig/wiki/EditorConfig-Properties#ideas-for-domain-specific-properties