Uh oh!
There was an error while loading. Please reload this page.
chore(sa): remove 11 dead ShellCheck disable directives - #963
Merged
Conversation
A directive for a warning that no longer fires is dead weight, and worse than that it is misleading -- it tells the next reader that the line below is doing something ShellCheck objects to, when it is not. This project has found stale ones twice before, in #928 and #947, both times by accident. Detected mechanically rather than by eye: strip every disable from a file, run ShellCheck, and see which declared codes actually come back. Two details made the difference between a correct list and a wrong one. The scan has to run in place. `.shellcheckrc` sets `source-path=src` with `external-sources=true`, so copying a file to a temp dir breaks source resolution and changes what SC2154 and SC2034 report -- exactly the codes under test. And it has to run under both gates. `make sa` uses `-xC` (follows `source`), CI's ludeeus/action-shellcheck does not, and the two see different things for cross-file symbols. A directive only counts as dead if it fires under neither. That distinction turned out not to change the list here, but assuming it would have been luck rather than method. Kept: the disables for SC1091, SC2155 and SC2016. Those never fire because both gates already exclude those codes globally, which is not the same as the warning being obsolete -- they still do their job for anyone running plain `shellcheck`. Removed (9 files, 11 lines): SC2153, SC2034 x3, SC2154 x3, SC2254, SC2030, SC2031, SC2295. Explanatory comments that sat above or beside a removed directive are kept where they still say something true. Every changed line is a comment; verified by filtering the diff. Suites match baseline exactly: 1630 sequential, 1589 parallel/simple/strict.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤔 Background
A
# shellcheck disable=for a warning that no longer fires is dead weight — and misleading, since it tells the next reader the line below is doing something ShellCheck objects to when it is not. This project has found stale ones twice before (#928, #947), both times by accident rather than by looking.💡 Changes
SC2153,SC2034×3,SC2154×3,SC2254,SC2030,SC2031,SC2295SC1091/SC2155/SC2016disable — those don't fire only because both gates exclude those codes globally, which is not the same as being obsolete; they still work for anyone running plainshellcheck🔍 How they were found
Mechanically, not by eye: strip every disable from a file, re-run ShellCheck, see which declared codes come back. Two details decided correct-vs-wrong:
/tmp..shellcheckrcsetssource-path=src+external-sources=true, so copying a file elsewhere breaks source resolution and changes whatSC2154/SC2034report — the exact codes under test.make saruns-xC(followssource); CI'sludeeus/action-shellcheckdoes not. They disagree on cross-file symbols, so a directive only counts as dead if it fires under neither. That didn't change the final list, but assuming it would have been luck.✅ Verification
Every changed line is a comment (diff filtered to confirm).
make sa·make lint· CI-mode ShellCheck (no-x, exact CI opts, clean) ·bash build.sh bin -v→✅ Build verified ✅.Suites match baseline exactly: 1630 sequential, 1589 parallel/simple/strict.