Add GitHub action to clear whitespace

[navidcy]

following the steps of NumericalEarth/Breeze.jl#406

[giordano]

Whitespace check found 584 issues:

Eh eh. I have an elisp script for cleaning things up, if you need help 😅

[navidcy]

@giordano I see that the action just checks but not actually clears the whitespaces.
How can I clear all whitespaces in this PR?

[navidcy]

You literally just answered before I asked..

[navidcy]

Feel free to share the script or just run it on the branch?

[glwagner]

can we just run this script automatically?

[navidcy]

Yeah... That'd be ideal. Like as part of the GitHub Action?

[giordano]

For the record the code I used is

(dolist (file (directory-files-recursively "/path/to/Oceananigans" "\\.\\(jl\\|md\\|sh\\)$"))
  (when (file-regular-p file)
    (with-temp-buffer
      (insert-file-contents file)
      ;; Force LF line ending
      (set-buffer-file-coding-system 'unix)
      ;; Add final newline in case it's missing.  If it's
      ;; extra it'll be cleaned up later.
      (goto-char (point-max))
      (insert "\n")
      ;; Replace non-breaking spaces
      (save-excursion
        (goto-char (point-min))
        (while (search-forward " " nil t)
          (replace-match " " nil t)))
      ;; Untabify
      (untabify (point-min) (point-max))
      ;; Clean up all trailing whitespaces
      (delete-trailing-whitespace)
      (write-region (point-min) (point-max) file))))

but not sure how to use it here, installing Emacs on the CI job is doable (apt install emacs-nox) but perhaps slightly overkill for this? But the more concrete point is that I'm not a huge fan of actions which forcibly edit the code in PRs. Would you like that?

[navidcy]

Let's leave it as is for now:
the action checks whether whitespaces have been introduced instead of automatically dealing with them.

[codecov]

Codecov Report

❌ Patch coverage is 65.62500% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.19%. Comparing base (88d6e56) to head (05ea9c9).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/DistributedComputations/distributed_fields.jl	0.00%	8 Missing ⚠️
...ceDissipationComputations/diffusive_dissipation.jl	62.50%	6 Missing ⚠️
...c/DistributedComputations/communication_buffers.jl	60.00%	4 Missing ⚠️
...OceananigansReactantExt/OceananigansReactantExt.jl	66.66%	3 Missing ⚠️
src/Operators/spacings_and_areas_and_volumes.jl	80.00%	2 Missing ⚠️
...losure_implementations/advective_skew_diffusion.jl	77.77%	2 Missing ⚠️
ext/OceananigansReactantExt/OutputReaders.jl	0.00%	1 Missing ⚠️
src/AbstractOperations/grid_metrics.jl	0.00%	1 Missing ⚠️
src/AbstractOperations/show_abstract_operations.jl	0.00%	1 Missing ⚠️
src/BuoyancyFormulations/buoyancy_force.jl	75.00%	1 Missing ⚠️
... and 4 more

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #5145   +/-   ##
=======================================
  Coverage   73.19%   73.19%           
=======================================
  Files         390      390           
  Lines       21847    21847           
=======================================
  Hits        15991    15991           
  Misses       5856     5856           

Flag	Coverage Δ
buildkite	68.76% <67.04%> (ø)
julia	68.76% <67.04%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[glwagner]

But the action will block PRs that violate the whitespace criterion. Without the script, it's not that easy to clear this failing test?

[giordano]

But the action will block PRs that violate the whitespace criterion.

It's technically non-blocking, but will have a red mark on PRs, yes.

Without the script, it's not that easy to clear this failing test?

I'd argue it takes active effort (or an LLM, they're somehow bad at the basic code formatting) to leave trailing whitespaces in hundreds of lines in a single PR. Most PRs will be clean, or have 1-2 lines which are easy to fix (the inline comments in the files changed tab will make it easy to see them).

I think a more effective approach is to a pre-commit hook, which resolves the most egregious issues at commit time for people who enable it. People who don't...will have to deal with the extra whitespaces themselves.

[glwagner]

But the action will block PRs that violate the whitespace criterion.

It's technically non-blocking, but will have a red mark on PRs, yes.

Without the script, it's not that easy to clear this failing test?

I'd argue it takes active effort (or an LLM, they're somehow bad at the basic code formatting) to leave trailing whitespaces in hundreds of lines in a single PR. Most PRs will be clean, or have 1-2 lines which are easy to fix (the inline comments in the files changed tab will make it easy to see them).

I think a more effective approach is to a pre-commit hook, which resolves the most egregious issues at commit time for people who enable it. People who don't...will have to deal with the extra whitespaces themselves.

Maybe I am the only one sloppy enough to have the issue. I think that I matter though 🥺

I'm not sure LLMs generate it. I sometimes get whitespace when I edit code through the github editor. I agree it is not a huge slowdown, but it is a slowdown nonetheless, especially if CI is expensive, it could delay merging by eg 90 min (2x the waiting time for CI)

[giordano]

I can work on a simple bash-based pre-commit hook to resolve the main issues (trailing white spaces on every line, non-breaking spaces, etc., those are easy to deal with sed), if you'd enable it.

[giordano]

I'm not sure LLMs generate it.

Every time I used Cursor I had to remove dozens of stupid trailing whitespaces, especially in blocks separated by blank lines, like

    some_code(x)
    # <-- This is a blank line, but Claude/Cursor _loves_ adding spaces here
    some_other_code(y)

Just by looking at the most recent failures of this check in Julia I quickly found https://github.com/JuliaLang/julia/actions/runs/20860074414/job/59937155255?pr=60621, which points to JuliaLang/julia@3ed531c#diff-5d5447f22a990c432721925eba28246028db844bd834cb358235c865f0558659, which, guess what, was created by Claude.

[navidcy]

I'm not sure LLMs generate it.

Every time I used Cursor I had to remove dozens of stupid trailing whitespaces, especially in blocks separated by blank lines, like

    some_code(x)
    # <-- This is a blank line, but Claude/Cursor _loves_ adding spaces here
    some_other_code(y)

Just by looking at the most recent failures of this check in Julia I quickly found https://github.com/JuliaLang/julia/actions/runs/20860074414/job/59937155255?pr=60621, which points to JuliaLang/julia@3ed531c#diff-5d5447f22a990c432721925eba28246028db844bd834cb358235c865f0558659, which, guess what, was created by Claude.

Could we add an instruction in AGENTS.md about that?

[glwagner]

I think it would be easy to add an instruction to remove trailing whitespace to AGENTS.md. We could also put the elisp script itself in the AGENTS.md.

[simone-silvestri]

I agree that the whitespace action is kind of annoying, I seem to always add whitespace but cannot find it! Would it be possible to implement a bot that commits to the PR the whitespace change when called (maybe through comments in the PR)? Or would it be very difficult to implement?
Btw, @giordano how do I run that script? Is it bash?

[giordano]

No, the script I shared above is elisp, you need Emacs to run it.

I'll try to prepare a simple pre-commit hook to clean up the main things, I believe that's more useful than a workflow (again, I don't think it's nice for workflows to modify source code in PRs, it also feels like a potential security issue, and I suspect it wouldn't work for PRs from forks)

[giordano]

@glwagner @simonesilvestri BTW, if you use VS Code or derivatives (which includes Cursor) you can also make your editor automatically trim trailing whitespace, see https://stackoverflow.com/a/53663494 or https://stackoverflow.com/a/30884298. That's basically what I do as well, just with a different editor.