configuration for white-space

[KrisThielemans]

a first step addressing #98 .

I have not run this on any existing files yet, but it does seem to work on new/changed files.

[KrisThielemans]

@dvolgyes @casperdcl could you have a quick look at this and see if I made any mistakes? Suggestions welcome!

[casperdcl]

I'd also recommend running & committing with something like git commit --author="clang format <noreply@github.com>" in this PR (or a follow-up PR) so as to not mess overly with git-blame

[KrisThielemans]

I'd also recommend running & committing with something like git commit --author="clang format noreply@github.com" in this PR (or a follow-up PR) so as to not mess overly with git-blame

great idea.

What would we recommend for contributors with their own branches, especially on-going PRs? Would the following be fine?

1. merge master
2. if no conflicts (or easy ones), go to 3, otherwise
   a. abort merge
   b. merge master with commit just before the white-space change on master
   c. sort out conflicts
   d. merge master again
3. run pre-commit run --all-files (or is it possible to run only on changed files?)
4. git commit --author="clang format <noreply@github.com>"

Relevant here I guess is @NikEfth 's experiment mentioned here #98 (comment)

[casperdcl]

1,2,3 seem fine.

4 isn't required. contributors in their own branches don't have to mess with --author at all. The diff for those commits will be minimal and likely only consist of conflict lines which they manually fixed, so should really be associated with their account and not some bot.

[casperdcl]

opened a small big patch KrisThielemans#3

[KrisThielemans]

4 isn't required.

ok

[casperdcl]

FYI I think you're using clang-format-6.0.

On clang-format-8, RawStringFormats.Delimiter isn't supported, and there are other errors with other versions (both older and newer) too. I'd recommend removing any keys you haven't explicitly set yourself to maximise compatibility...

[KrisThielemans]

would be great if we could add some editor config settings that (roughly) fit the .clang-format. Otherwise it'll be a pain for most people. (edit file, commit, reload file), as far as I can see all the editor plug-ins require you to press some buttons to get the formatting to be ok (but i haven't tried any)

[KrisThielemans]

I'd recommend removing any keys you haven't explicitly set yourself to maximise compatibility...

ah. I thought I was being smart by output the default config and then editing it. Are you saying it'd be better to just add the keywords I listed in #98 (comment)? seems to make sense

[casperdcl]

better to just add the keywords I listed

exactly

[casperdcl]

it'll be a pain for most people. (edit file, commit, reload file)

if your editor (as it should) supports live-reloading files which are edited externally then it's not a problem. Attempting to commit will

• if pre-commit is installed, trigger clang-format
• if clang-format changes files, the commit is aborted, allowing the user to add the changed files
• if no files are changed, the commit succeeed

Some editors do support .clang-format via plugins etc but it's probably best to link to http://clang.llvm.org/docs/ClangFormat.html for that sort of thing

[KrisThielemans]

better to just add the keywords I listed

exactly

I've done that now. However, for whatever reason I had to uninstall the pre-commit hook, as it made garbage of the .clang-format file (messed about with EOL stuff). Any idea why/how we can prevent that?

[casperdcl]

No, that would've been because you ran pre-commit before my regex fix (so it was reformatting the .clang-format file since .c appears in the filename)

[ashgillman]

@KrisThielemans

I've just done a test of my own to make sure I'm understanding everything here.

1. Pulled locally UCL/STIR/master, branched to clean-master
2. Rebased White space config KrisThielemans/STIR-1#3 onto master, except for the final commit with all the changes - I wanted to run myself
3. I had to add an extra change: ashgillman@bd9b642
4. Ran the pre-commit command, and committed as noreply@github.com
5. Pulled locally Time Of Flight reconstruction  #304/head (named to clean-tof_sino_UCL)
   • I tried merging master at this point - prior to cleaning. But it had a lot of conflicts on whitespace.
6. Instead, I also rebased White space config KrisThielemans/STIR-1#3 here so I could run the clang-format
7. ran the pre-commit command
8. now I merged master using git merge clean-master -s recursive -X theirs
   • This merge strategy should mean that whenever there are identical changes in each branch (from the clang-format) it will use the one from clean-master
9. It worked! Just one file that was deleted since the last merge of master (print_rdf_singles.cxx) that is was confused about.
10. Made a Pull request for clean-tof_sino_UCL onto clean-master

You can see the results here: ashgillman#3
11.6k lines added vs 15,8k lines added here: #304

[ashgillman]

Okay, I take this back - line 8 is wrong.

Instead - use git merge clean-master -Xignore-space-change clean-master. There should be merge conflicts, but this tricks git into ignoring any that differ by whitespace. Hopefully, there are none - this allows git to ignore identical changes in both from clang-format though

[KrisThielemans]

that's great news. I suggest we merge this in stages:

1. @casperdcl 's PR here (without his last commit)
2. this PR onto master
3. separate PR that runs clang-format

[ashgillman]

I agree

[ashgillman]

If you need to work with a branch that was forked prior to in inclusion of clang-format, you will need to temporarily disable/uninstall pre-commit again:

    pre-commit uninstall

[casperdcl]

Or git commit --no-verify

[ashgillman]

Build Error is just due to a timeout I believe

[KrisThielemans]

@ashgillman want to do a PR with these? Or use the "suggestions" button.