Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the whitespace checking/fixing scripts against edge cases (invalid Unicode output, missing/empty results artifacts, and incorrect diff invocation) and adjusts the wrapper behavior to avoid running the fixer when the checker produced no usable output.
Changes:
- Replaced Unicode warning output with ASCII-safe text in the whitespace checker.
- Ensured
check-results.logexists even when there are zero commits to report, and switched parsing to use the in-memory$log. - Adjusted diff invocation in the fixer and gated running the fixer based on checker outcome/artifacts.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Build/Agent/fix-whitespace.ps1 | Updates the fallback git diff call used to determine files to fix |
| Build/Agent/check-whitespace.ps1 | Makes results logging resilient to empty output and replaces Unicode warning text |
| Build/Agent/check-and-fix-whitespace.ps1 | Cleans stale results and gates fixer execution based on checker outcome |
26d1fa8 to
a5bbb76
Compare
jasonleenaylor
left a comment
There was a problem hiding this comment.
Did you check this on a clean commit? (good commit message and no white space issues)
@jasonleenaylor reviewed 10 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on johnml1135).
|
9 lines of bash replaced by 4 lines of powershell plus a 69 line powershell script for the same functionality doesn't feel like an improvement. |
Summary
Build/AgenthelperWhy
The original whitespace fix addressed real edge cases, but the helper scripts had drifted into a split local-vs-CI model:
check-results.logComment body cannot be blankwarning on successThis PR keeps the original whitespace fixes, makes the whitespace flow easier to reason about, removes duplicate bash maintenance, and clarifies the contract for both checks:
Build/AgentLanding place for the checks
Whitespace
.github/workflows/check-whitespace.ymlBuild/Agent/check-whitespace.ps1Build/Agent/check-and-fix-whitespace.ps1Build/Agent/fix-whitespace.ps1Why it is split this way:
check-whitespace.ps1owns detection and status reporting.0= no whitespace issues2= whitespace issues found1= checker failed unexpectedlycheck-results.log, but that file is now an artifact for parsing and inspection rather than the hidden control signal for the wrapper.check-and-fix-whitespace.ps1uses the exit code to decide whether it is safe to run the fixer.fix-whitespace.ps1usescheck-results.logwhen available to target the reported files, and otherwise falls back to branch-introduced changes frommerge-base(origin/main, HEAD)...HEAD.Commit messages
.github/workflows/CommitMessage.ymlBuild/Agent/commit-messages.ps1Build/Agent/publish-commit-message-summary.ps1Why it is split this way:
commit-messages.ps1owns the gitlint invocation and returns gitlint's exit code.check_results.logso the same human-readable output can be reused in the job summary and sticky PR comment.publish-commit-message-summary.ps1owns result normalization and GitHub Actions output/summary formatting, so that logic is testable in PowerShell instead of being embedded in YAML.success()/failure().only_update: true, so successful runs hide an existing sticky comment if one exists and do nothing otherwise.Changes
Whitespace tooling
check-results.logas hidden control flow for the wrappercheck-results.logdeterministic by recreating it at the start of the checkermerge-base(origin/main, HEAD)...HEAD, so it targets branch-introduced changes rather than drift on the moving tip ofmainCommit-message tooling
pwshgitlinton demand when it is not already availablecheck_results.logdeterministic by recreating it at the start of the checkerBuild/Agent/publish-commit-message-summary.ps1No problems found.Comment body cannot be blankwarning on successful runs with no existing sticky commentCleanup
Validation
./Build/Agent/commit-messages.ps1./Build/Agent/check-whitespace.ps1./Build/Agent/check-and-fix-whitespace.ps1./Build/Agent/publish-commit-message-summary.ps1for empty-success and populated-failure inputsCI: Full local check./Build/Agent/commit-messages.ps1after creating follow-up commitsThis change is
This change is