ci: add workflow to lint whitespace

[ee7]

Enforce that every git-tracked, non-empty text file:

• has no trailing whitespace
• ends in exactly one newline

The script and workflow are taken from the configlet repo.

The script output and factoring can be improved, but it does at least exit non-zero when it's supposed to.

---

Follow-up from #204

[ee7]

This repo doesn't have any binary files, but it's generally important to include the -I option - otherwise the script can exit with an error for "trailing whitespace" in a binary file.

There is at least one action in the GitHub Marketplace that doesn't handle that correctly.

Docs for git grep: https://git-scm.com/docs/git-grep

[ee7]

Deliberately allows an empty file.

That doesn't matter for this repo, but we might want that feature if this script ends up running in a bunch of repos.

[ErikSchierboom]

ends in exactly one newline

See #204 (comment) for context on why this might not yet be what we want.

[ee7]

If removing one of the final newlines there does indeed affect the generated file, can we have one final newline and tweak the file generation process instead? Relying on a double final newline seems a bit fragile, since so many people configure their editor to trim whitespace. And such a change is easy to miss, if a reviewer doesn't know/remember in a year's time that this file needs a double final newline.

Otherwise, I could allow-list the problematic file. But it would be nice to keep the script generic, to minimize the diff if we wanted to use it in several repos.

(Alternative route that I'm trying to avoid for now: create a whitespace linting action with a customizable ignore list).

[ErikSchierboom]

If removing one of the final newlines there does indeed affect the generated file, can we have one final newline and tweak the file generation process instead?

That would be best. @SaschaMann knows the generation process best.

[SaschaMann]

If removing one of the final newlines there does indeed affect the generated file, can we have one final newline and tweak the file generation process instead?

The file generation process is file-agnostic and for some files it's required to append the appends without an empty line inbetween. We could add a special case for labels.yml but imho having inconsistent behaviour between appendable files feels like a good way to shoot ourselves in the foot in the future.

That said, if you both agree that we should special-case the file, I'm happy to go along. We'd have to add it here:

org-wide-files/scripts/apply-track-config.jl

Lines 42 to 47 in 8b1b6e4

	# Append file
	open(joinpath("track-repo", file), "a") do io
	write(io, append_content)
	@info "Appended $file" append_content
	end

[SaschaMann]

And such a change is easy to miss, if a reviewer doesn't know/remember in a year's time that this file needs a double final newline.

The obvious solution for this would be to add labels.yml to CODEOWNERS across the entire org. Perhaps even with a new team called DONT-EDIT-ORG-WIDE-FILES or something along those lines.

[ee7]

The file generation process is file-agnostic and for some files it's required to append the appends without an empty line inbetween.

Oh. Please remind me: what are our appendable files right now? And do we plan to have more?

inconsistent behaviour between appendable files feels like a good way to shoot ourselves in the foot in the future.

I agree.

And such a change is easy to miss, if a reviewer doesn't know/remember in a year's time that this file needs a double final newline.

The obvious solution for this would be to add labels.yml to CODEOWNERS across the entire org.

By "reviewer" I meant "reviewer in the exercism/org-wide-files repo". If the double final newline is currently necessary, we might accidentally convert it to a single newline in this repo later (in a PR that performs other changes in that file). Adding the file to CODEOWNERS doesn't protect us from that, right? Even if we had:

requested a review from exercism@MAKE_SURE_THIS_PR_DOESNT_REMOVE_THE_DOUBLE_FINAL_NEWLINE_FROM_global-files/.github/labels.yml as code owner

We also can't add a comment for it in the file itself like "the below double newline is deliberate, and must not be converted to a single final newline" (since I presume that comment would end up in the downstream files).

Possible hack: amend this PR to add a special case for that file alone - require it to have exactly two final newlines. Is that the least bad option so far?

[SaschaMann]

Oh. Please remind me: what are our appendable files right now? And do we plan to have more?

You can find them in config.toml:

org-wide-files/config.toml

Lines 1 to 5 in 8b1b6e4

	# List of files that can be appended in the target repos
	appendable_files = [
	"CODE_OF_CONDUCT.md",
	".github/labels.yml",
	]

.gitattributes is also planned: #19.

By "reviewer" I meant "reviewer in the exercism/org-wide-files repo". If the double final newline is currently necessary, we might accidentally convert it to a single newline in this repo later (in a PR that performs other changes in that file). Adding the file to CODEOWNERS doesn't protect us from that, right? Even if we had:

Ah sorry, I misunderstood. I thought you meant downstream repos. Codeowners would not really protect us in that case. A warning might help use realise it but I'm sure I'd ignore it after seeing it twice.

We also can't add a comment for it in the file itself like "the below double newline is deliberate, and must not be converted to a single final newline" (since I presume that comment would end up in the downstream files).

Possible hack: amend this PR to add a special case for that file alone - require it to have exactly two final newlines. Is that the least bad option so far?

I'd be fine with either of those. I don't think it's a problem if a comment would make it downstream, as long as we add a short explanation why it can be ignored by most people. Having a custom check for it seems fine, too.

[SaschaMann]

We could also add it as an option to config.toml. That way, we'd be reminded of the differences when adding new appendable files.

[ee7]

You can find them in config.toml
[...]
.gitattributes is also planned

I see. Thanks.

Aside, out of curiosity: what's the reasoning for CODE_OF_CONDUCT.md being appendable? Is that written somewhere? We truly want to support e.g. a track repo having its own additional rules in the code of conduct?

[SaschaMann]

Aside, out of curiosity: what's the reasoning for CODE_OF_CONDUCT.md being appendable? Is that written somewhere? We truly want to support e.g. a track repo having its own additional rules in the code of conduct?

The Julia track has an addendum as a precaution because the behaviour described in it is unfortunately something that actually occurs (even if I'm not aware of it happening on Exercism yet): https://github.com/exercism/julia/blob/main/CODE_OF_CONDUCT.md#addendum

Ideally this should live on the site somewhere but back then Jeremy decided it should go in the track repo for now.

[ErikSchierboom]

We could also add it as an option to config.toml. That way, we'd be reminded of the differences when adding new appendable files.

This would be my preference

[ee7]

Rebased on main.

Previously in this PR:

There is trailing whitespace on the below lines:
.github/labels.yml:# 
global-files/.github/labels.yml:# 
Multiple newlines at end of file: global-files/.github/labels.yml

Now (after f760ee1):

There is trailing whitespace on the below lines:
.github/labels.yml:# 
global-files/.github/labels.yml:# 

[ErikSchierboom]

Okay, the other whitespace PR is merged, so a rebase should fix CI.

[ee7]

Rebased on main again, but there's still:

There is trailing whitespace on the below lines:
.github/labels.yml:# 

Can we trim it by triggering a PR in this repo, or just do it manually?

[ErikSchierboom]

Can we trim it by triggering a PR in this repo, or just do it manually?

I think manually would be fine.

[ee7]

I think manually would be fine.

Created #216

[ErikSchierboom]

Merged.

[ErikSchierboom]

Nice!

[ErikSchierboom]

Now that we have everything merged, we could trigger the sync workflow for the select few repositories that held off on merging due to the trailing whitespace. IIRC, they included exercism/elixir, exercism/configlet and exercism/problem-specifications. Am I missing any?

[ee7]

Also exercism/nim and exercism/nim-test-runner, please.

I don't mind what we do for repos that aren't the 5 mentioned above, but there is at least also exercism/tooling-orchestrator#40

[ErikSchierboom]

https://github.com/exercism/org-wide-files/actions/runs/2269577877 started