configuration for white-space #724

KrisThielemans · 2020-10-22T08:39:17Z

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 · 2020-10-22T08:41:36Z

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

documentation/devel/git-hooks.md

casperdcl · 2020-10-22T11:45:59Z

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 · 2020-10-22T12:26:45Z

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?

merge master
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
run pre-commit run --all-files (or is it possible to run only on changed files?)
git commit --author="clang format <noreply@github.com>"

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

casperdcl · 2020-10-22T12:32:23Z

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.

Co-authored-by: Casper da Costa-Luis <casper.dcl@physics.org>

casperdcl · 2020-10-22T13:13:35Z

opened a ~~small~~ big patch KrisThielemans#3

KrisThielemans · 2020-10-22T13:54:38Z

4 isn't required.

ok

casperdcl · 2020-10-22T13:56:34Z

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 · 2020-10-22T13:58:15Z

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 · 2020-10-22T14:04:40Z

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 · 2020-10-22T14:06:51Z

better to just add the keywords I listed

exactly

casperdcl · 2020-10-22T14:11:34Z

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

Only use the non-default ones for the style, as otherwise it causes problems with different versions of clang-format

KrisThielemans · 2020-10-22T22:33:08Z

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 · 2020-10-22T22:40:10Z

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 · 2021-05-26T04:01:17Z

@KrisThielemans

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

Pulled locally UCL/STIR/master, branched to clean-master
Rebased White space config KrisThielemans/STIR-1#3 onto master, except for the final commit with all the changes - I wanted to run myself
I had to add an extra change: ashgillman@bd9b642
Ran the pre-commit command, and committed as noreply@github.com
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.
Instead, I also rebased White space config KrisThielemans/STIR-1#3 here so I could run the clang-format
ran the pre-commit command
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
It worked! Just one file that was deleted since the last merge of master (print_rdf_singles.cxx) that is was confused about.
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 · 2021-05-26T04:29:31Z

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 · 2021-05-26T07:57:59Z

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

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

ashgillman · 2021-05-26T08:57:24Z

I agree

* fix pre-commit * add CI test

.pre-commit-config.yaml

ashgillman · 2021-05-27T01:39:18Z

documentation/devel/git-hooks.md

+```sh
+cd /whereever/STIR
+pre-commit install
+```


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

Or git commit --no-verify

ashgillman · 2021-05-27T01:40:43Z

Build Error is just due to a timeout I believe

KrisThielemans · 2021-05-27T06:07:25Z

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

documentation/devel/git-hooks.md

Co-authored-by: Casper da Costa-Luis <casper.dcl@physics.org> Co-authored-by: Ashley Gillman <gillmanash@gmail.com>

KrisThielemans requested a review from ashgillman October 22, 2020 08:40

KrisThielemans added enhancement in-progress labels Oct 22, 2020

KrisThielemans added this to the v5.0 milestone Oct 22, 2020

KrisThielemans linked an issue Oct 22, 2020 that may be closed by this pull request

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

Closed

add .clang-format, precommit hooks and minimal doc

15d726f

KrisThielemans force-pushed the white-space-config branch from a97383b to 15d726f Compare October 22, 2020 10:25

casperdcl reviewed Oct 22, 2020

View reviewed changes

documentation/devel/git-hooks.md Outdated Show resolved Hide resolved

casperdcl reviewed Oct 22, 2020

View reviewed changes

documentation/devel/git-hooks.md Outdated Show resolved Hide resolved

casperdcl reviewed Oct 22, 2020

View reviewed changes

documentation/devel/git-hooks.md Outdated Show resolved Hide resolved

remove use of sudo for pip instructions

c10c303

Co-authored-by: Casper da Costa-Luis <casper.dcl@physics.org>

reduced keys .clang-format [ci skip]

922031d

Only use the non-default ones for the style, as otherwise it causes problems with different versions of clang-format

This was referenced Apr 26, 2021

use override attribute for virtual functions #827

Closed

Modernize use override #868

Closed

ashgillman mentioned this pull request May 26, 2021

ToF with Whitespace ashgillman/STIR#3

Open

ashgillman mentioned this pull request May 26, 2021

Time Of Flight reconstruction #304

Merged

White space config (#3)

0810225

* fix pre-commit * add CI test

ashgillman requested changes May 27, 2021

View reviewed changes

ashgillman reviewed May 27, 2021

View reviewed changes

documentation/devel/git-hooks.md Show resolved Hide resolved

Apply suggestions from code review in UCL#724 [ci skip]

43b98e9

Co-authored-by: Casper da Costa-Luis <casper.dcl@physics.org> Co-authored-by: Ashley Gillman <gillmanash@gmail.com>

KrisThielemans merged commit 7fdb13a into UCL:master May 27, 2021

KrisThielemans deleted the white-space-config branch May 27, 2021 12:47

ashgillman mentioned this pull request Aug 17, 2021

TIme of Flight - clean #918

Closed

KrisThielemans mentioned this pull request Nov 17, 2023

enforce Python style #1289

Open

KrisThielemans mentioned this pull request Feb 6, 2024

update and run clang-format #1368

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

configuration for white-space #724

configuration for white-space #724

KrisThielemans commented Oct 22, 2020

KrisThielemans commented Oct 22, 2020

casperdcl commented Oct 22, 2020 •

edited

Loading

KrisThielemans commented Oct 22, 2020

casperdcl commented Oct 22, 2020 •

edited

Loading

casperdcl commented Oct 22, 2020 •

edited

Loading

KrisThielemans commented Oct 22, 2020

casperdcl commented Oct 22, 2020 •

edited

Loading

KrisThielemans commented Oct 22, 2020

KrisThielemans commented Oct 22, 2020

casperdcl commented Oct 22, 2020

casperdcl commented Oct 22, 2020

KrisThielemans commented Oct 22, 2020

casperdcl commented Oct 22, 2020 •

edited

Loading

ashgillman commented May 26, 2021

ashgillman commented May 26, 2021

KrisThielemans commented May 26, 2021

ashgillman commented May 26, 2021

ashgillman May 27, 2021

casperdcl May 27, 2021

ashgillman commented May 27, 2021

KrisThielemans commented May 27, 2021

configuration for white-space #724

configuration for white-space #724

Conversation

KrisThielemans commented Oct 22, 2020

KrisThielemans commented Oct 22, 2020

casperdcl commented Oct 22, 2020 • edited Loading

KrisThielemans commented Oct 22, 2020

casperdcl commented Oct 22, 2020 • edited Loading

casperdcl commented Oct 22, 2020 • edited Loading

KrisThielemans commented Oct 22, 2020

casperdcl commented Oct 22, 2020 • edited Loading

KrisThielemans commented Oct 22, 2020

KrisThielemans commented Oct 22, 2020

casperdcl commented Oct 22, 2020

casperdcl commented Oct 22, 2020

KrisThielemans commented Oct 22, 2020

casperdcl commented Oct 22, 2020 • edited Loading

ashgillman commented May 26, 2021

ashgillman commented May 26, 2021

KrisThielemans commented May 26, 2021

ashgillman commented May 26, 2021

ashgillman May 27, 2021

Choose a reason for hiding this comment

casperdcl May 27, 2021

Choose a reason for hiding this comment

ashgillman commented May 27, 2021

KrisThielemans commented May 27, 2021

casperdcl commented Oct 22, 2020 •

edited

Loading

casperdcl commented Oct 22, 2020 •

edited

Loading

casperdcl commented Oct 22, 2020 •

edited

Loading

casperdcl commented Oct 22, 2020 •

edited

Loading

casperdcl commented Oct 22, 2020 •

edited

Loading