feat(health): add a severity tier so a check can report without failing validation - #58
Open
dev-noaman wants to merge 1 commit into
Open
Conversation
…ng validation ValidationResult.ok was `len(issues) == 0`, so every check was necessarily fatal. That left no way to surface a condition a maintainer should see without also failing `validate`, `ensure_valid`, and the workflows gated on them -- `sync` among them. Any informational check added under the old contract would break every wiki it fired on. ValidationIssue gains `severity: IssueSeverity`, defaulting to ERROR, and ValidationResult gains `errors`/`warnings` views. `ok` now means "no error-severity issues". Behaviour is unchanged for everything that exists today: all 13 construction sites omit severity and therefore stay ERROR, so no current check changes classification and no exit code moves. This adds the capability without exercising it -- deciding whether any existing category (unused_sources, empty_topics, orphans) is really a warning is a policy call for maintainers, deliberately left alone here. Two details worth flagging: - `validation_failure_message` now describes the first ERROR rather than the first issue, so a preceding warning cannot become the headline of a failure it did not cause. - `render_validate` prints warnings in their own section and still prints them when validation passes. A warning nobody sees is not a warning. Tests cover the default, that a warning leaves ok true, that an error still fails, and that warnings do not mask errors in the failure message.
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
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.
Adds a severity tier to
ValidationIssueso a check can report a condition without failing validation. Follow-up to #56/#57, but independent of them — this branch is cut frommainand the two can merge in either order.Why
ValidationResult.okis currently:so every check is necessarily fatal. There is no way to surface something a maintainer should see without also failing
validate,ensure_valid, and everything gated on them —syncincluded.That came up concretely in #57. I offered to add a
healthnotice for pages skipped because they sit under a reserved directory, then withdrew it: under the current contract, an informational issue firing on the bundledmanual/— present in every wiki — would flipvalidatenon-zero and failsynceverywhere. The diagnostic would have caused a worse bug than the one it explained.The missing piece is not that particular check. It is the tier.
What changes
ValidationResult.okbecomes "no error-severity issues".ValidationResult.errors/.warningsexpose the two views.validation_failure_messagereports the first error, so a preceding warning cannot become the headline of a failure it did not cause.render_validateprints warnings in their own section, including when validation passes. A warning nobody sees is not a warning.Backward compatibility
This is the part I most want reviewed.
All 13 existing
ValidationIssueconstruction sites omitseverity, so they default toERRORand behave exactly as before. No current check changes classification, no exit code moves, and the JSON gains a field rather than changing one.No existing category is reclassified. Whether
unused_sources,empty_topicsororphansare really warnings is a policy decision that belongs to maintainers, so this PR adds the capability without exercising it. Happy to follow up with a reclassification if you have views.Behaviour
Note the middle line: the message names the error, not the warning that precedes it.
Tests
Four tests in
tests/test_validate.pycovering the default severity, a warning leavingoktrue, an error still failing, and warnings not masking errors in the failure message.Checks
Per
CONTRIBUTING.md:uv run pytest— failing set identical before and after (14 both ways on this machine; all pre-existing and unrelated — 5 are macOSlaunchdtests on Windows, one is a sqliterepositories.nameisolation issue).tests/test_validate.pygoes 6 → 10 passed.uv run ruff check .— All checks passeduv build— sdist and wheel builtIf you'd rather not
This is a change to a public contract, so it is reasonable to want it shaped differently — a separate
noticescollection instead of a field, say, or severity confined to the health report rather thanValidationIssue. I'm happy to rework or drop it; the motivating case is only that a diagnostic should be able to inform without blocking.