contributing: Add clang-format shell script #2732

nilason · 2023-01-04T20:45:49Z

Adds a grass_clang_format.sh script.

Usage:

If you have clang-format in PATH, execute for complete source formatting:
./utils/grass_clang_format.sh
Setting 'GRASS_CLANG_FORMAT' to explicitly set clang-format binary (its naming can depend on how various
distributions name it):
GRASS_CLANG_FORMAT="clang-format-15" ./utils/grass_clang_format.sh
It is also possible to format the content in a (one) given directory:
./utils/grass_clang_format.sh ./lib/raster

utils/grass_clang_format.sh

neteler · 2023-01-08T12:11:17Z

I have tested the script on my Fedora 36 box (in order to update an existing PR) but find doesn't find anything yet:

# attempting to update PR cmake_prep_infinity_nan
bash -x grass_clang_format.sh
+ '[' -z '' ']'
+ case "$-" in
+ __lmod_vx=x
+ '[' -n x ']'
+ set +x
Shell debugging temporarily silenced: export LMOD_SH_DBG_ON=1 for this output (/usr/share/lmod/lmod/init/bash)
Shell debugging restarted
+ unset __lmod_vx
+ set -eu
+ req_cf_v=15
+ '[' '!' -f .clang-format ']'
+ '[' '!' -f ../.clang-format ']'
+ '[' -z ']'
+ fmt=clang-format
+ clang-format --version
++ clang-format --version
+ clang_version_full='clang-format version 15.0.6 (https://github.com/ssciwr/clang-format-wheel a427ab6a8d0bbc93a80b82a99afe9996c06377b4)'
++ echo 'clang-format version 15.0.6 (https://github.com/ssciwr/clang-format-wheel a427ab6a8d0bbc93a80b82a99afe9996c06377b4)'
++ cut -f3 '-d '
++ cut -f1 -d.
+ clang_version=15
+ '[' 15 -lt 15 ']'
+ case "$(uname)" in
++ uname
+ find=find
+ '[' 0 -eq 1 ']'
+ dir=.
+ find . -type f -iregex '.*\.(cpp|hpp|c|h)' -exec clang-format -i '{}' +

git diff
# ... no changes.

# checking only find:
find . -type f -iregex '.*\.(cpp|hpp|c|h)'
# ... no result.

find --version
find (GNU findutils) 4.9.0

It seems the regex needs another tweak for (Linux) GNU findutils.

nilason · 2023-01-08T13:09:54Z

It seems the regex needs another tweak for (Linux) GNU findutils.

Seems GNU find uses emacs regex by default. Please try:

find . -type f -regextype posix-extended -iregex '.*\.(cpp|hpp|c|h)'

neteler · 2023-01-08T20:13:58Z

Yes, the modified version appears to work with GNU find:

find . -type f -regextype posix-extended -iregex '.*\.(cpp|hpp|c|h)'  | wc -l
3357

nilason · 2023-01-08T20:29:11Z

Yes, the modified version appears to work with GNU find:
find . -type f -regextype posix-extended -iregex '.*\.(cpp|hpp|c|h)'  | wc -l
3357

Good! Updated accordingly.

neteler · 2023-01-08T20:32:30Z

Almost there... (GNU find here):

sh -x grass_clang_format.sh db
+ set -eu
...
+ find db -type f '-regextype posix-extended -iregex' '.*\.(cpp|hpp|c|h)' -exec clang-format -i '{}' +
find: unknown predicate `-regextype posix-extended -iregex'

neteler · 2023-01-08T20:45:56Z

With un-quoting ${regex} it works for me.

nilason · 2023-01-08T20:58:45Z

With un-quoting ${regex} it works for me.

I have the same problems using zsh -x ./utils/grass_clang_format.sh, but it works fine running just ./utils/grass_clang_format.sh.

Unquoting $regex causes shellcheck to complain:

shellcheck ./utils/grass_clang_format.sh

In ./utils/grass_clang_format.sh line 85:
${find} "${dir}" -type f ${regex} '.*\.(cpp|hpp|c|h)' -exec \
                         ^------^ SC2086 (info): Double quote to prevent globbing and word splitting.

neteler · 2023-01-08T21:16:20Z

I have the same problems using zsh -x ./utils/grass_clang_format.sh, but it works fine running just ./utils/grass_clang_format.sh.

Here it doesn't:

utils/grass_clang_format.sh db
find: unknown predicate `-regextype posix-extended -iregex'

nilason · 2023-01-09T08:30:15Z

I have the same problems using zsh -x ./utils/grass_clang_format.sh, but it works fine running just ./utils/grass_clang_format.sh.

Here it doesn't:
utils/grass_clang_format.sh db
find: unknown predicate `-regextype posix-extended -iregex'

Always these minor differences. Let's use standard find and skip regular expression. Probably the safest way.
Updated.

neteler · 2023-01-09T14:04:59Z

Great, now it works also for me!

Just a glitch I spotted (shall I open a new issue?) - the header formatting is a bit distorted. This happens in the cmake related PR which I am trying to update:

diff --git a/db/db.columns/main.c b/db/db.columns/main.c
index 3cdda5384e..9e7400462b 100644
--- a/db/db.columns/main.c
+++ b/db/db.columns/main.c
@@ -3,9 +3,9 @@
  *
  * MODULE:       db.columns
  * AUTHOR(S):    Radim Blazek <radim.blazek gmail.com> (original contributor)
- *               Glynn Clements <glynn gclements.plus.com>, Markus Neteler <neteler itc.it>
- * PURPOSE:      list the column names for a table
- * COPYRIGHT:    (C) 2002-2006 by the GRASS Development Team
+ *               Glynn Clements <glynn gclements.plus.com>, Markus Neteler
+ *<neteler itc.it> PURPOSE:      list the column names for a table COPYRIGHT:
+ *(C) 2002-2006 by the GRASS Development Team
  *
  *               This program is free software under the GNU General Public
  *               License (>=v2). Read the file COPYING that comes with GRASS
@@ -19,17 +19,13 @@
 #include <grass/dbmi.h>
 #include <grass/glocale.h>

nilason · 2023-01-09T15:22:36Z

Great, now it works also for me!

That's good news!

Just a glitch I spotted (shall I open a new issue?) - the header formatting is a bit distorted. This happens in the cmake related PR which I am trying to update:
diff --git a/db/db.columns/main.c b/db/db.columns/main.c
...

I went through all files manually (in addition to clang-format) in the big formatting commits, removing remaining tabs in comments and fixed the above mentioned headers (hopefully all) where it was needed. Because of this, clang-format isn't always enough, for some old PRs.

If there are no changes to the header in the PR, then you should leave this hunk as is (as it is good in main).
(Out of the hat, I don't know a git-y way to do that, but you can always manually copy that part from the main version before merging/rebasing.)

nilason · 2023-01-09T15:35:27Z

Great, now it works also for me!

That's good news!
Just a glitch I spotted (shall I open a new issue?) - the header formatting is a bit distorted. This happens in the cmake related PR which I am trying to update:
diff --git a/db/db.columns/main.c b/db/db.columns/main.c
...
I went through all files manually (in addition to clang-format) in the big formatting commits, removing remaining tabs in comments and fixed the above mentioned headers (hopefully all) where it was needed. Because of this, clang-format isn't always enough, for some old PRs.

If there are no changes to the header in the PR, then you should leave this hunk as is (as it is good in main). (Out of the hat, I don't know a git-y way to do that, but you can always manually copy that part from the main version before merging/rebasing.)

I should add: It is best to only apply clang-format to the files in a particular PR that are modified (and not to the whole source dir).

neteler · 2023-01-09T17:53:05Z

I should add: It is best to only apply clang-format to the files in a particular PR that are modified (and not to the whole source dir).

Good point.

BTW: This would be a trick to easily list the files changed in a PR (source: https://stackoverflow.com/a/68682405/452464):

# list files changed in PR 2681
gh pr view 2681 --json files --jq '.files.[].path'

Only those then would undergo the clang-format -i <file> beautification.

wenzeslaus · 2023-02-14T19:27:42Z

Is this still useful with pre-commit? (Just trying to clean up the 8.3.0 milestone.)

nilason · 2023-02-14T20:02:40Z

As long as pre-commit is not a requirement (and/or we use pre-commit-ci), then this might be useful for those who for whatever reason do not want to install pre-commit (and simplify their lives :). My intention was to replace the now obsolete grass_indent.sh.
I have no strong opinion on this, either we bin it or merge it now, no need to bump milestone on this.

wenzeslaus · 2023-02-14T20:17:20Z

Okay, let's make this available. We want to make it easy, so an alternative is good. Can you please delete the old indent files?

This reverts commit 1cd1dde.

neteler · 2023-02-14T20:47:23Z

Is this still useful with pre-commit? (Just trying to clean up the 8.3.0 milestone.)

It is useful since pre-commit is, as far as I can see, yet heavily underused (also note to self).

wenzeslaus · 2023-02-14T20:59:01Z

You can run pre-commit run clang-format --all-files instead of the script and you need to install something in both cases, but if you Python environment is not in perfect order, installing pre-commit will can be harder than installing clang-format. That's what the slight advantage of a script is. Neither will force people to use that. That's what only CI can do.

The clang-format shell script replaces the grass_indent.sh and grass_indent_ALL.sh scripts, which are both hereby removed.

add clang-format script

656e42d

nilason added the enhancement New feature or request label Jan 4, 2023

nilason added this to the 8.3.0 milestone Jan 4, 2023

minor fixes

4dc07a9

nilason requested review from marisn and neteler January 6, 2023 19:05

neteler reviewed Jan 8, 2023

View reviewed changes

utils/grass_clang_format.sh Outdated Show resolved Hide resolved

use posix extended on gnu, check for .clang-format in source only

1837bb6

use standard find

3e8c82c

remove grass_indent[_ALL].sh

1cd1dde

wenzeslaus approved these changes Feb 14, 2023

View reviewed changes

nilason added 3 commits February 14, 2023 21:32

Revert "remove grass_indent[_ALL].sh"

0f1b787

This reverts commit 1cd1dde.

Merge branch 'main' into replace-grass-indent

e922b9a

remove grass_indent[_ALL].sh again

bd3c0d4

nilason merged commit 4746c46 into OSGeo:main Feb 15, 2023

nilason deleted the replace-grass-indent branch February 15, 2023 08:22

ninsbl pushed a commit to ninsbl/grass that referenced this pull request Feb 17, 2023

Add clang-format shell script (OSGeo#2732)

294c4fb

The clang-format shell script replaces the grass_indent.sh and grass_indent_ALL.sh scripts, which are both hereby removed.

wenzeslaus changed the title ~~Add clang-format shell script~~ contributing: Add clang-format shell script Jun 6, 2023

neteler pushed a commit to nilason/grass that referenced this pull request Nov 7, 2023

Add clang-format shell script (OSGeo#2732)

0590a87

The clang-format shell script replaces the grass_indent.sh and grass_indent_ALL.sh scripts, which are both hereby removed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

contributing: Add clang-format shell script #2732

contributing: Add clang-format shell script #2732

nilason commented Jan 4, 2023

neteler commented Jan 8, 2023

nilason commented Jan 8, 2023

neteler commented Jan 8, 2023

nilason commented Jan 8, 2023

neteler commented Jan 8, 2023

neteler commented Jan 8, 2023

nilason commented Jan 8, 2023

neteler commented Jan 8, 2023

nilason commented Jan 9, 2023

neteler commented Jan 9, 2023

nilason commented Jan 9, 2023

nilason commented Jan 9, 2023

neteler commented Jan 9, 2023

wenzeslaus commented Feb 14, 2023

nilason commented Feb 14, 2023

wenzeslaus commented Feb 14, 2023

neteler commented Feb 14, 2023

wenzeslaus commented Feb 14, 2023

contributing: Add clang-format shell script #2732

contributing: Add clang-format shell script #2732

Conversation

nilason commented Jan 4, 2023

neteler commented Jan 8, 2023

nilason commented Jan 8, 2023

neteler commented Jan 8, 2023

nilason commented Jan 8, 2023

neteler commented Jan 8, 2023

neteler commented Jan 8, 2023

nilason commented Jan 8, 2023

neteler commented Jan 8, 2023

nilason commented Jan 9, 2023

neteler commented Jan 9, 2023

nilason commented Jan 9, 2023

nilason commented Jan 9, 2023

neteler commented Jan 9, 2023

wenzeslaus commented Feb 14, 2023

nilason commented Feb 14, 2023

wenzeslaus commented Feb 14, 2023

neteler commented Feb 14, 2023

wenzeslaus commented Feb 14, 2023