Skip to content

Replace the opt-in /format-code workflow with always-on style enforcement #4854

Description

@allister-beamable

Summary

/format-code is the only style enforcement we have, it is opt-in, nobody invokes it, and it has been broken for long enough that nobody noticed. I propose we retire it and replace it with always-on mechanical enforcement, introduced one rule at a time so that each renormalization pass is a single reviewable commit rather than a permanent low-grade source of diff noise.

The current mechanism does not work

.github/workflows/formatCodeOnComment.yml runs only when someone comments /format-code on a pull request. Nothing runs on push or on pull request.

It is also broken in at least three independent ways:

  1. No root tool manifest. format-code.sh runs dotnet tool restore from the repository root. There is no .config/dotnet-tools.json there:

    Cannot find a manifest file. The list of searched paths:
      <repo>/.config/dotnet-tools.json
      <repo>/dotnet-tools.json
      ...
    

    Manifest lookup walks up from the working directory, never down, so the manifests under cli/cli/, stress-tests/standalone-microservice/, and beam-native-mobile/Samples/ReactNative/ are never found. Both format steps fail. format-code.sh has no set -e, so it proceeds to dotnet tool run dotnet-format and fails there instead.

  2. Archived tool. The pinned tool is dotnet-format 5.1.250801, the standalone package that was archived when dotnet format moved into the SDK in .NET 6.

  3. ::set-output. The workflow sets steps.format-changes.outputs.any through ::set-output, deprecated by GitHub in October 2022. Even with a working manifest, the commit-back step would likely never fire.

Corroborating evidence that nobody has noticed: the last 100 formatCodeOnComment runs are all skipped. The command has not been invoked in months.

The design problem

Expecting contributors to remember an undiscoverable slash command is not a workable enforcement model, and the evidence above is that it never was. There are two honest positions:

  • No enforcement. Delete the workflow and .editorconfig's formatting rules, and accept whatever lands.
  • Always-on enforcement that cannot be accidentally dodged.

What we have now is the worst of both: a declared style nobody checks, so .editorconfig reads as aspirational and the codebase drifts. I favor the second position.

Scale of the debt

Across 3190 tracked .cs files:

Problem Extent
Trailing whitespace 1684 files, 15779 lines
Line endings 1377 all-CRLF, 1524 all-LF, 287 internally mixed
Indentation not yet measured

Over half the trailing-whitespace debt is in generated code. 238 .gs.cs files account for 8376 of the 15779 lines, leaving 7403 in hand-written code. cli/beamable.common/Runtime/OpenApi/Models.gs.cs and its Unity counterpart alone contribute 6726.

This is the most important constraint on any cleanup: fixing generated files by hand accomplishes nothing, because the next generateApi run reverts it. Either the generators emit clean output or generated paths are excluded from enforcement.

Proposed sequence

One rule per pass. Each pass is a single mechanical commit, landed when no large branch is in flight, so that git blame has exactly one line of noise to skip and reviewers of unrelated work never see formatting churn.

  • Step 0 — decide on generated code. Either fix the emitters or exclude generated paths from all enforcement. Nothing else can proceed cleanly first.
  • Step 1 — trailing whitespace. The safest possible starting point: no semantic risk, no editor disagreement, nothing to argue about.
  • Step 2 — line endings, normalizing to LF. .gitattributes with *.cs text eol=lf, .editorconfig updated from end_of_line = crlf, then git add --renormalize .. This one needs coordination, since contributors have differing core.autocrlf settings and that is the likely cause of the 287 mixed files today.
  • Step 3 — indentation. Tabs per .editorconfig, preserving tab-plus-space continuation alignment, which is deliberate and should not be collapsed.
  • Step 4 — var versus explicit types, and the remaining .editorconfig style rules. Lowest value and highest bikeshed potential, so it goes last.

Enforcement to be added alongside Step 1 so that the debt cannot grow back: dotnet format --verify-no-changes as a required pull request check, scoped to the rules adopted so far and widened as each step lands.

Notes

  • .editorconfig currently declares indent_style = tab and end_of_line = crlf. Step 2 changes the latter. Whatever we do not intend to enforce should be removed from .editorconfig rather than left there misleading people.
  • A working check would have caught both style problems found in review on Fix content deletion races and surface nested validation errors (#4845) #4853, which is what prompted this.
  • Whether enforcement auto-fixes and commits back to the branch, or merely fails the check and asks the contributor to run a command, is worth deciding explicitly. Committing back to pull request branches has known friction with re-runs and force pushes, so failing the check is probably the saner default.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestneeds-discussionThis issue or PR requires discussion before it gets merged

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions