Fix the triaged detekt baseline violations - #70
Merged
Conversation
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.
Why
All 357 baseline entries were reviewed individually and sorted into fix / keep / adjust-the-rule. This PR contains only the fix group — 50 entries where the rule was right and the code was genuinely worse for the violation.
Nothing here is a mechanical edit to satisfy detekt. Every change stands on its own.
What changed
Stale callbacks in restartable effects (14) — the most valuable group. Lambda parameters were captured inside
LaunchedEffect/DisposableEffect, so when the lambda changed the running effect kept calling the old one. Each now goes throughrememberUpdatedState. This is a correctness fix, not style: an out-of-dateonCreatedoronProjectSelectedsilently navigates using a stale closure.GitHub OAuth field names (6) —
GitHubDeviceFlowAuthenticatorhad snake_case@Serializablefields plus six hand-written accessor properties existing purely to expose camelCase names:Two names for every field. Replaced with
@SerialName("device_code") val deviceCode, deleting all six accessors. Every@SerialNamestring was checked character-by-character against the old property name —Jsonis configured withignoreUnknownKeys = true, so a typo would fail silently as a null field rather than throwing.Idiomatic error handling (11) —
throw IllegalStateException(...)→error(...),if (!x) throw ...→check(x) { }/require(x) { }. Only where the transformation is exact; sites carrying acauseor rethrowing inside acatchwere left alone.Swallowed exceptions (2) —
KeystoreExceptionnow preserves its underlying cause instead of discarding it.Composable parameter order (3) —
modifiermoved to first-optional position.GitDiffRouteturned out to be a different violation than expected: a requiredonBackafter an optionalcommitId. Its caller was converted to named arguments.Hygiene (14) — 7 missing trailing newlines, a wildcard import, an explicit
itlambda parameter, an unused parameter, an empty else block.An extra improvement found during review
EditorRoutehit exactly 60 lines after therememberUpdatedStateadditions. Rather than shave a line, I looked at what was in it: a 12-line hand-rolledDisposableEffect+LifecycleEventObserver— duplicated almost verbatim inGitPanelScreen.Both are replaced with the official
LifecycleEventEffectfromlifecycle-runtime-compose:That removes ~20 lines and a duplicated concept. No new dependency — both files already import
collectAsStateWithLifecyclefrom the same artifact.Behavior
Three deliberate behaviour changes, all fixes:
KeystoreExceptionpreserves its cause.Everything else is behaviour-preserving. The OAuth JSON wire format is unchanged.
Deliberately NOT fixed
Reviewed and kept baselined, with reasons:
TooGenericExceptionCaught(18) — all boundary code (JGit,ApkInstaller,ArtifactDownloader,RemoteBuildSystem). Catching broadly at an I/O boundary and mapping to a domain error is correct design.ComplexCondition(17) andLoopWithTooManyJumpStatements(13) — all parsers, lexers and the terminal emulator. Multi-clause conditions and multi-exit loops are inherent to tokenizers.ParameterNaming(20) — compose-rules wants present tense (onFontSizeChange, notonFontSizeChanged). That is churn across composable + interaction listener + ViewModel for convention alone, and several (onCloned,onCreated,onConflictPathOpened) describe genuinely completed events where past tense is correct.ThrowsCount(5 remaining) — boundary code with several distinct failure modes.MultipleEmitters(4),ModifierMissing(5) — changing these alters layout structure or adds parameters to route composables that fill the screen.EmptyFunctionBlocksites are required no-op interface overrides in tests;WelcomeBulletPreviewis used by preview tooling.MatchingDeclarationName(36) is handled separately as a rule-configuration change.Tests
No new tests. These are corrections to existing behaviour with no new logic; the full existing suite passes.
Verification
./gradlew test— all modules pass../gradlew :app:compileDebugKotlin— passes../gradlew detekt— passes.@SerialNamevalues verified against the original snake_case names.Baseline
Every module's baseline regenerated — 50 removed, 0 added, project total 357 → 307.
Two removals were side effects rather than direct targets, and both were checked:
ForbiddenImport:HubSectionHeader.kt— a stale entry for a file deleted in Share the section header composable #56.ThrowsCount:downloadWithChecksum— converting its five explicit throws tocheck()/error()genuinely reduced the throw count.