[CI] Add format ignore file #2803

FranzBusch · 2024-07-24T13:36:05Z

Motivation

Some repositories may want to exclude files from being format checked.

Modification

This PR moves the formatting into a separate script and adds an optional .swiftformatignore file that can be placed at the repo root similar to the .licenseignore file.

Result

Repos can now decide what files are ignored when running the formatter.

# Motivation Some repositories may want to exclude files from being format checked. # Modification This PR moves the formatting into a separate script and adds an optional `.swiftformatignore` file that can be placed at the repo root similar to the `.licenseignore` file. # Result Repos can now decide what files are ignored when running the formatter.

FranzBusch · 2024-07-24T13:38:01Z

Format check is expected to fail since I moved the inline scripts into a file.

czechboy0

Not loving .swiftformatignore as a name, I think .swift-format-ignore would be more readable, but approving either way :)

FranzBusch · 2024-07-24T13:52:24Z

Not loving .swiftformatignore as a name, I think .swift-format-ignore would be more readable, but approving either way :)

I was going back and forth. But decided for the former because of .gitignore and .licenseignore. I briefly thought about .swift-formatignore but that just looks even worse

weissi · 2024-07-24T14:10:33Z

scripts/check-swift-format.sh

+
+log "Running swift format lint..."
+
+git ls-files -z '*.swift' $excluded_files | xargs -0 swift format lint --strict --parallel


quoting error here

These are intentionally without quotes since I to split on spaces. Otherwise this exclude list doesn't work

This is not how you do that in shell (will break with spaces in the file names)... The correct syntax is

files=( foo 'foo bar' buz ) echo "${files[@]}"

will do what you want

Maybe it would have made more sense to use git ls-files -z ... and then use grep -f to make use of the exclude file?

Or git ls-files $(cat IGNOREFILE | xargs -I% printf ":(exclude)% ")

git ls-files $(cat IGNOREFILE | xargs -I% printf ":(exclude)% ")

This has two quoting errors, one in bash and one with xargs. You may be able to leverage printf "...%q" which outputs it quoted. But I think the right solution here is

# -X <file>, --exclude-from=<file> # Read exclude patterns from <file>; 1 per line. git ls-files -X IGNOREFILE

Yeah, doesn't work as advertised :) Only applies to files in the index, which is a bit of a bummer.

🤯 infuriating!

weissi · 2024-07-24T14:10:39Z

scripts/check-swift-format.sh

+
+excluded_files=""
+
+if [ -f .swiftformatignore ]; then


prefer [[

My understanding was that [[ isn't portable so I preferred [.

That's an incorrect understanding. You're writing bash (as you should and as indicated by your shebang) not "POSIX shell".

Once you have bash, [[ is a built-in which means absolute maximum compatibility. If you type [ you're invoking the program called /bin/[:

$ ls -la /bin/[ -rwxr-xr-x 1 root wheel 101456 12 Jul 21:18 /bin/[

weissi · 2024-07-24T14:11:01Z

scripts/check-swift-format.sh

+##===----------------------------------------------------------------------===##
+set -euo pipefail
+
+log() { printf -- "** %s\n" "$*" >&2; }


can we run these scripts through shellcheck? Quoting errors etc are avoidable and can be checked by tooling

I am running a shell check locally already but yes we should setup a GH action for it

Shell check would've spotted the above quoting error, right?

indeed

$ echo -e 'files="foo bar"\necho $files' | shellcheck --shell=bash - In - line 2: echo $files ^----^ SC2086 (info): Double quote to prevent globbing and word splitting. Did you mean: echo "$files" For more information: https://www.shellcheck.net/wiki/SC2086 -- Double quote to prevent globbing ...

Shell check found two quoting warnings not errors for line 29 and line 33 https://www.shellcheck.net/wiki/SC2086.

Capitalised variables are bad as they have unknown, unintended side effects. The space of lower case vars is reserved (by POSIX) for app code.

Where we do want side effects (like with GIT_PAGER=....) they're great. That's exactly how they're meant to be used:

UPPER_CASE_STUFF -> side effects in other programs

lower_case_stuff -> my everyday variables that I do not want to go to other programs

I'm not arguing with you here. I have long used uppercase variables in my bash scripts, but I have heard your arguments against them and plan to incorporate them in the future.

FWIW, I don't think the side-effect argument is actually true, unless the variable is exported, but I'm still planning on stopping this practice in my own scripts because it helps with local reasoning.

Right, for some reason this isn't widely known. But I think it's well documented:

stackoverflow: https://stackoverflow.com/questions/673055/correct-bash-and-shell-script-variable-capitalization

posix: https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html

And yes, the exported thing is correct (unless you prefix a command/script) but these things happen by accident. And as you point out, it's just hard to reason about it. If you assign to a variable called IFS I don't know if you're meaning to overwrite IFS for say read or if you want a variable called IFS. If you name it ifs=123 on the other hand I know what you mean.

We could also use local variables more, I try to do that everywhere except when needing to export vars.

Yep, I readily accept that I have fallen into the trap of shouty-case for all my scripting variables. I'm not alone, I know. But I always knew when they were and weren't exported so they didn't affect behaviour, and I've learned to not name my variables to clash with ones that I'll need to affect behaviour. But, like I've already said, I've started to lowercase—we can all grow.

weissi · 2024-07-24T14:42:53Z

scripts/check-swift-format.sh

+
+log "Checking for modified files..."
+
+GIT_PAGER= git diff --exit-code '*.swift'


the space here is also incorrect, right?

No I set the GIT_PAGER to null intentionally.

gotcha. Might be nice to be explicit here: GIT_PAGER='' git diff ...

This is very idiomatic shell already.

If we're gonna make it explicit, I'd rather env -u GIT_PAGER git diff ....

IMHO that's obfuscation. I'd suggest to ask a few people what happens if you do

FOO= hello world

vs.

FOO='' hello world

In my mind, the first one is confusing, it might be an assignment of hello world to FOO= or it might be starting hello with parameter world and env var FOO=''.

The second one is obvious.

Did you mean the use of env is obfuscation? If anything it's clearer and has different semantics. It unsets the variable, as opposed to setting it to the empty string.

I concede that for setting a variable to the empty string, FOO='' is clearer than FOO=.

I don't think it's quite as much of an alarm bell as you make out, and shellcheck doesn't complain, but I'm happy for it to change if it irks you.

Yes, I was referring to env. If this is common knowledge that env -u FOO hello world runs hello world, then I'm completely happy with that of course. As am I with

unset FOO hello world

or

FOO='' hello world

because I think they convey the meaning pretty well.

But out of all the issues, this really is the most minuscule one. Feel free to leave GIT_PAGER= git diff, it just reads a little off in my head at least.

FranzBusch requested a review from simonjbeaumont July 24, 2024 13:36

simonjbeaumont approved these changes Jul 24, 2024

View reviewed changes

czechboy0 approved these changes Jul 24, 2024

View reviewed changes

glbrntt merged commit 541a900 into main Jul 24, 2024
25 of 26 checks passed

glbrntt deleted the fb-format-ignore-file branch July 24, 2024 13:58

glbrntt added the semver/none No version bump required. label Jul 24, 2024

weissi reviewed Jul 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CI] Add format ignore file #2803

[CI] Add format ignore file #2803

FranzBusch commented Jul 24, 2024

FranzBusch commented Jul 24, 2024

czechboy0 left a comment

FranzBusch commented Jul 24, 2024

weissi Jul 24, 2024

FranzBusch Jul 24, 2024

weissi Jul 24, 2024

simonjbeaumont Jul 25, 2024

weissi Jul 25, 2024

simonjbeaumont Jul 25, 2024

weissi Jul 25, 2024

weissi Jul 24, 2024

FranzBusch Jul 24, 2024

weissi Jul 24, 2024

weissi Jul 24, 2024

FranzBusch Jul 24, 2024

weissi Jul 24, 2024

weissi Jul 24, 2024

FranzBusch Jul 24, 2024

weissi Jul 25, 2024

simonjbeaumont Jul 25, 2024

weissi Jul 25, 2024

czechboy0 Jul 26, 2024

simonjbeaumont Jul 26, 2024

weissi Jul 24, 2024

FranzBusch Jul 24, 2024 •

edited

Loading

weissi Jul 24, 2024

simonjbeaumont Jul 25, 2024

weissi Jul 25, 2024

simonjbeaumont Jul 25, 2024

weissi Jul 25, 2024


		log "Running swift format lint..."

		git ls-files -z '*.swift' $excluded_files \| xargs -0 swift format lint --strict --parallel


		log "Checking for modified files..."

		GIT_PAGER= git diff --exit-code '*.swift'

[CI] Add format ignore file #2803

[CI] Add format ignore file #2803

Conversation

FranzBusch commented Jul 24, 2024

Motivation

Modification

Result

FranzBusch commented Jul 24, 2024

czechboy0 left a comment

Choose a reason for hiding this comment

FranzBusch commented Jul 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranzBusch Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranzBusch Jul 24, 2024 •

edited

Loading