Clear the standing compiler warnings and gate on them in CI - #316
Merged
Conversation
Six is enough noise that a seventh — a real one — scrolls past unread. Only one of them was a latent defect; the rest are recorded here so nobody re-litigates them later. The real one: BundleSchemaGoldenTest called File.parentFile.mkdirs() on a receiver Kotlin now types as nullable. It cannot actually be null for the relative path used there, but the warning is on its way to becoming an error, and a golden-refresh path that NPEs would be found at the worst moment. The other five were deliberate: four `Unit` literals standing in for empty branches in Compose `when` slots, and one Json format rebuilt inside a test body. The `Unit`s said "nothing happens here", which is what an empty branch already says, so the branches are now empty; the format is built once per class. No behaviour change. Not adding allWarningsAsErrors along with this: it would lock in a clean build today at the cost of breaking CI on the next Kotlin or AGP bump, which is the maintainer's call rather than a side effect of a cleanup.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
The module now compiles clean, so make that the enforced state rather than a snapshot. `-PvpnhideWarningsAsErrors=true` on the CI Gradle invocations, matching how -PvpnhideEmulatorX86 is already wired; off by default, because a warning mid-edit is information, not a reason to stop a local build. The usual objection to allWarningsAsErrors is that a toolchain bump turns unrelated new warnings into a red CI on someone else's PR. That does not apply here: every input that decides what warns is pinned — Gradle by the wrapper, AGP/Kotlin/Compose in libs.versions.toml, the NDK by ndkVersion, and the JDK by the CI container image. New warnings can only arrive with a deliberate version bump, on the PR that does the bumping, where they belong. Verified both directions: reintroducing the nullable-receiver warning fixed in the previous commit passes without the flag and fails with it.
okhsunrog
changed the title
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.
The module compiled with six warnings, which is enough noise that a seventh — a real one — would scroll past unread. Only one was a latent defect. With the module clean, CI now enforces that state.
BundleSchemaGoldenTestcalledFile.parentFile.mkdirs()on a receiver Kotlin now types as nullable. It cannot be null for the relative path used there, so nothing is broken today, but the warning is on its way to becoming a hard error and the failure mode would be a golden-refresh path that NPEs.The other five were deliberate, and are documented in the commit rather than left to be re-litigated: four
Unitliterals standing in for empty branches in Composewhenslots, and oneJsonformat rebuilt inside a test body. None needed an@Suppressin the end — written plainly, they stop warning by construction.allWarningsAsErrorsis gated behind-PvpnhideWarningsAsErrors=true, passed by the CI Gradle invocations and off by default, so a warning mid-edit doesn't stop a local build. The usual objection — a toolchain bump reddening someone else's PR — does not apply here: every input that decides what warns is pinned (Gradle by the wrapper, AGP/Kotlin/Compose inlibs.versions.toml, the NDK byndkVersion, the JDK by the CI container image), so new warnings can only arrive on the PR that does the bumping.golden.parentFile?.mkdirs()Unitno-op branch bodies inFullResetDialog,LegacyImportUi,DiagnosticsScreenDiagnosticReportTest'sJsonformat once per class instead of per callvpnhideWarningsAsErrorsproperty and pass it from the lint/test and APK-build CI stepsTesting: verified both directions — reintroducing the nullable-receiver warning passes without the flag and fails the build with it.