The markup check says something, or says nothing - #383
Merged
Merged
Conversation
#378 made the check run in CI for the first time. It then reported 130 warnings, which is a wall of output nobody reads and a real finding nobody can see. Five of them were real. `active_npub`, `identity_kind_label`, `logout_confirming`, `previews_state` and `previews_action` were declared and referenced nowhere at all, in this file or in the tests: plain accessors left behind when settings controls moved into the Zig view and took different accessors with them. The warning offers bind it, remove it, or declare it; for these the answer was remove, and they are gone. The remaining 125 are expected rather than defects. This app draws its own view in Zig and the one markup file is 24 lines of join screen, so most of the model really is read only by Zig. They are declared in the two `view_unbound` lists now, with a note saying why, which is what those lists are for. The point of clearing them is not tidiness. At 130 lines a genuine finding is indistinguishable from the noise around it, which is how five dead functions sat there. At zero, a newly added field or Msg tag is the only thing the check says. So the thing worth checking was that it can still fail. Removing one declared tag brings it back by name, one warning and nothing else, and putting it back returns to silence. A check silenced into permanent silence would be worse than the noise it replaced. One trap on the way, and the guard from #378 caught it exactly as intended: editing this file after emitting the contract leaves the contract stale, and a stale contract makes the typed half skip and report zero warnings for the wrong reason. The zero here was verified with a freshly built contract, and the CI step now fails rather than believing that. Closes #382.
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.
Closes #382. Follow-up to #378, which made the check run in CI for the first time.
It then reported 130 warnings, which is a wall nobody reads and a real finding nobody can see.
Five were real
Declared and referenced nowhere at all, in
src/main.zigorsrc/tests.zig:active_npub·identity_kind_label·logout_confirming·previews_state·previews_actionPlain accessors, left behind when settings controls moved into the Zig view and took different accessors with them. The warning offers bind it, remove it, or declare it. For these the answer was remove.
The other 125 are expected
This app draws its own view in Zig and
src/onboarding.nativeis 24 lines of join screen, so most of the model genuinely is read only by Zig. They are declared in the twoview_unboundlists with a note saying why, which is what those lists are for.Why clear them at all
Not tidiness. At 130 lines a genuine finding is indistinguishable from the noise around it, which is exactly how five dead functions sat there unnoticed. At zero, a newly added field or Msg tag is the only thing the check says.
So the thing worth checking is that it can still fail:
A check silenced into permanent silence is worse than the noise it replaced.
The guard from #378 earned its keep immediately
Editing this file after emitting the contract leaves the contract stale, and a stale contract makes the typed half skip and report zero warnings for the wrong reason. I hit that mid-change. The zero above was verified against a freshly built contract, and the CI step now fails rather than believing it.