refactor(diag): unify severity prefixes on one helper per crate - #1217
Merged
Conversation
Diagnostic prefixes were written in six places across two crates in three spellings: `PreprocDiagnostic`'s five `Display` arms, two open-coded CSV sites, `warn_non_utf8_path`, the code-climate empty-path skip, and the walker's non-regular-file skip. Each reads as correct in isolation, which is why they accumulated. The prefix now belongs to whichever layer presents the message: - `PreprocDiagnostic::Display` renders the bare message, and `bca preproc` routes it through the CLI's `diag::warn`. - The library gets its own `diag::warn`, the counterpart to the CLI ladder added in #609, and every library stderr warning goes through it. - CSV joins the five other output formats on `warn_non_utf8_path`, losing its duplicated wording as a side effect. Two deliberate output changes: the `IncludeCycle` block no longer ends in a newline (it stacked with the caller's own and printed a blank line after every cycle), and the CSV warning takes the shared helper's wording. Verified otherwise byte-identical by diffing `bca preproc` stderr on a fixture across the change. `utils/check-diagnostic-prefix.py` (`make check-diagnostic-prefix`, wired into lint / pre-commit / ci and the pre-commit hooks) blocks a seventh site, with self-tests covering both directions. The baseline refresh carries `write_code_climate`'s halstead.effort +0.2% plus three entries that had gone stale on main. Fixes #1199
Review follow-ups on #1199: - The gate flagged capitalised literals inside multi-line raw-string fixtures, where neither `diag-prefix-ok` position is reachable — both land inside the fixture, so the marker would alter the text a metric test measures. Skip those interiors. Verified across all 559 tracked files that the skip hides zero would-be hits. - The naive raw-string opener read `strip_suffix(b"\r")` and `Ruby::R => "r",` as opening a raw string and skipped to the next quote — a false *clean*, the outcome the gate exists to prevent. Excluding `"` and `\` from the lookbehind cuts candidate multi-line opens from 99 to 27, all genuine fixtures. Both shapes are pinned. - `re.search` reported one offender per line, so a two-offender line was under-reported and the header undercounted. Use `finditer`. - An escaped inner quote (`"he said \"Error: no\""`) was flagged, contradicting the documented prose exemption. Rejected by lookbehind. `walk.rs`'s two explicit-path notices carried the `bca: warning:` double prefix that #609 removed elsewhere — named as "the old redundant double prefix" in `paths_discovery.rs:466` — which made this PR's own AGENTS.md rule false on arrival. Migrated onto `diag::warn`, with the three assertions updated and a guard pinning its absence. The CHANGELOG carried #1198's entry claiming a lowercase prefix and byte-compatibility that this change supersedes; both would have shipped in one release. Folded into one entry stating the end state.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1217 +/- ##
==========================================
+ Coverage 98.32% 98.33% +0.01%
==========================================
Files 277 278 +1
Lines 71955 71979 +24
Branches 71525 71549 +24
==========================================
+ Hits 70747 70779 +32
+ Misses 796 790 -6
+ Partials 412 410 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Codecov put patch coverage at 92.5% against a 98.32% project figure, naming three lines. Both gaps are real branches, not instrumentation noise, and neither had a test before this PR touched the lines. `write_code_climate`'s empty-repo-relative-path skip was covered only at the `normalize_path` unit level, so nothing pinned what the writer does with the `None` — skip the finding and continue, rather than abandon the document or emit a finding with an empty `location.path`. A second well-formed offender is what tells those apart. Replacing the `else` arm with `unwrap_or_default()` fails it. `PreprocDiagnostic::IncludeCycle` is the only variant that writes more than once, so it is the only one that can swallow a formatter error, and `to_string()` cannot catch that because a `String` sink never fails. The new test drives it through a sink that fails on the Nth write, for every N up to a *measured* total, so the assertion stays exactly "no write error is swallowed" if the impl is later rewritten to emit a different number of writes. Turning either `?` into `let _ =` fails it. Verified with `cargo llvm-cov`: preproc.rs:89,100 and code_climate.rs:94,96,98 all leave the uncovered-region set.
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes #1199.
Severity prefixes moved off the message producers and onto the layer
that presents them, so
warning:is written in exactly one place percrate:
diag::warnin the CLI (the #609 ladder) and a newsrc/diag.rsin the library.What changed
PreprocDiagnostic::Displayrenders the bare message;bca preprocroutes it through
diag::warn.src/diag.rs(pub(crate) fn warn), and everylibrary stderr warning goes through it — including the two that were
already lowercase, which had been correct by coincidence.
warn_non_utf8_path,dropping its duplicated wording.
write_csv/write_csv_aggregatekeep their signatures, so nothing is deferred to 3.0.
walk.rs's two explicit-path notices shed thebca: warning:doubleprefix that style(cli): unify diagnostic prefixes across the binary #609 removed elsewhere.
utils/check-diagnostic-prefix.py(+ 26 self-tests), wiredinto
make lint/pre-commit/ci, the pre-commit hooks, and anexplicit CI step.
The issue undercounted
It names three capitalised library sites; a workspace sweep found five.
src/output/code_climate.rs(empty repo-relative path) andsrc/concurrent_files.rs(not a regular file) were missing, as waswalk.rs's double prefix in the CLI. That is precisely the failuremode the gate exists to stop, already realised.
User-visible output changes
Three, all intended and all in the changelog:
IncludeCycleblock no longer ends in a newline, so it is nolonger followed by a blank line. It was an artifact of
writeln!ina
Displayimpl stacking with the caller's own newline.walk.rs's two notices readwarning:instead ofbca: warning:.PreprocDiagnosticis a public export, andSTABILITY.md:158— "theexact wording of
Displayoutput is not [stable]" — is what clearsthis. Embedders that captured these and printed them verbatim must
now add their own prefix.
Everything else is byte-identical:
bca preprocstderr was captured ona fixture before and after, and the only line that differs is the blank
one above.
Verification
Every new or changed test was proven to fail against the pre-fix code
by perturbing production, not by reading:
eprintln!trips the prefix assertion;restoring the
writeln!trips the blank-line assertion —independently.
continueinto areturnfails the newaggregate_skips_only_the_non_utf8_file. That branch had no test, anda single-file document that skips its only file is byte-identical to
one that bails, so the existing test structurally could not cover it.
splitlines()the gate reports a line number one too high past aU+2028 in a literal.
Two gate bugs found and fixed in review, both of the false-clean kind
this gate exists to prevent: a naive raw-string opener read
strip_suffix(b"\r")andRuby::R => "r",as opening a raw string andskipped to the next quote, and
re.searchreported only the firstoffender per line. Measured across all 559 tracked files, the
multi-line-fixture skip now hides zero would-be hits.
non_utf8_path_skips_data_rowsalso had its#[cfg(unix)]moved froman inner block onto the
fn: the old form compiled to an empty — andtherefore passing — body on Windows (
.claude/rules/testing.md).Baseline
write_code_climate'shalstead.effortmoved +0.2% past its recordedvalue, so
.bca-baseline.tomlis refreshed in the same commit. Therefresh also dropped three entries that had gone stale on
main.make pre-commit:BCA_GATE: pass.