From 46ccec3439e2f9e67f342edcc6a67cf6b4107882 Mon Sep 17 00:00:00 2001 From: Kgothatso Ngako Date: Sat, 3 Oct 2026 00:25:16 +0200 Subject: [PATCH] fix(scripts): stop m3-touch-targets.py reading comments as code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A KDoc line on the dialect picker's row composable, explaining that it used `Card(onClick = …)` rather than a `Modifier.clickable`, was counted as a bare clickable chain. The budget is zero, so a sentence about the rule broke `./gradlew check`. The comment was reworded to get around it, which was the wrong fix -- the sentence was right -- and it is restored here. Not a new rule. m3-spacing-positions.py skips `\s*(//|\*|/\*)` and m3-title-case.py skips the same three prefixes as a startswith tuple; only this checker was missing the two block-comment ones, having been written with `//` alone. m3-audit.sh explains the whole problem at NOT_A_COMMENT, where a note about a hardcoded colour once counted as a colour. The same function had the mirror bug, which is the dangerous direction. It looked for minimumInteractiveComponentSize in a two-line window without stripping comments, so a comment above a real bare `.clickable` that merely mentioned the modifier would have excused it. A false positive is noise; a silently excused offender is a miss. Both lines of the window are now read as code only. Checked against six cases: a bare clickable is still caught, a guard on the same line and a guard on the line above still excuse one, a starred KDoc no longer counts, and a real offender under a comment naming the modifier is now caught. Counts for the tree are unchanged -- 0 before and after -- which was worth establishing, since a broken detector would also report zero. A prefix test rather than block tracking, and the limitation is written on the function: a `/* ... */ `body line not starting with `*` still reads as code. There are 539 block comments under ui/ and not one such line, because every KDoc here stars its body, and three identical rules spelled three ways would be worse than one limitation on the record. Co-Authored-By: Claude Opus 5 --- .../AddDialectsFromCatalogueScreen.kt | 8 ++--- docs/african-dialect-catalogue.md | 34 ++++++++++++++----- docs/scripts/m3-touch-targets.py | 29 ++++++++++++++-- 3 files changed, 55 insertions(+), 16 deletions(-) diff --git a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/AddDialectsFromCatalogueScreen.kt b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/AddDialectsFromCatalogueScreen.kt index 66a63737..8c9b478d 100644 --- a/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/AddDialectsFromCatalogueScreen.kt +++ b/composeApp/src/commonMain/kotlin/press/mantra/compose/ui/composable/AddDialectsFromCatalogueScreen.kt @@ -382,12 +382,10 @@ private fun CatalogueList( /** * One catalogue entry. * - * A clickable `Card` rather than a bare clickable modifier, so the whole row is - * the target and is 48dp without anybody having to remember + * `Card(onClick = …)` rather than a `Modifier.clickable`, so the whole row is the + * target and it is 48dp without anybody having to remember * `minimumInteractiveComponentSize` -- the same shape `SelectSubgroupAdminsScreen` - * uses for picking members. (Spelled this way round because - * `m3-touch-targets.py` reads comments as code; see the note in m3-audit.sh on - * the colour check, which hit the same thing.) + * uses for picking members. */ @Composable private fun LanguageRow( diff --git a/docs/african-dialect-catalogue.md b/docs/african-dialect-catalogue.md index 8353f132..fdec86ce 100644 --- a/docs/african-dialect-catalogue.md +++ b/docs/african-dialect-catalogue.md @@ -502,16 +502,34 @@ translate into, and fifty of them arriving unasked is a statement nobody made. | `CatalogueRow.rowsOf` computes the three standings | it derives them from `missingFrom` | two functions deciding what is proposable is one too many. MISSING is whatever that function says, and held-versus-in-flight is a label on what it already excluded | | a wave "is small rather than just legal" — under a quarter of `MAX_PROPOSAL_BYTES` | at most 602 bytes *per entry* | the catalogue encodes to 16,832 bytes, which is 25.7% of the budget — the quarter was a guess and it failed. The per-entry cost is the thing that would actually drift, and it does not move when a language is added | -### One thing found on the way +### One thing found on the way, and fixed -`docs/scripts/m3-touch-targets.py` reads comments as code. A KDoc line on the -picker's row composable explaining that it used a clickable `Card` "rather than a -`Modifier.clickable`" was counted as a bare clickable chain, which breaks a zero -budget and so breaks `./gradlew check`. The comment was reworded to get around -it, which is the wrong fix -- the sentence was right. +`docs/scripts/m3-touch-targets.py` read comments as code. A KDoc line on the +picker's row composable explaining that it used `Card(onClick = …)` "rather than +a `Modifier.clickable`" was counted as a bare clickable chain, which breaks a +zero budget and so breaks `./gradlew check`. The comment was first reworded to +get around it, which was the wrong fix -- the sentence was right. `m3-audit.sh` already solves this for the colour check and says why, at `NOT_A_COMMENT`: "a note *about* a hardcoded colour counts as one -- which happened the first time a call site was fixed and the commit explained what it -had replaced." The touch-target checker wants the same treatment, and -`m3-title-case.py` should be looked at too, since it scans whole files. +had replaced." + +Fixed on 2026-10-03, and the KDoc restored. Two things were worth knowing while +looking: + + - **The other two checkers already had it.** `m3-spacing-positions.py` skips + `\s*(//|\*|/\*)` and `m3-title-case.py` skips the same three prefixes as a + `startswith` tuple. Only the touch-target one was missing the two block-comment + prefixes, so the fix is the house rule rather than a new one. + - **The same function had the mirror bug, which is the dangerous direction.** It + looked for `minimumInteractiveComponentSize` in a two-line window without + stripping comments, so a comment above a real bare `.clickable` that merely + mentioned the modifier would have excused it. A false positive is noise; that + is a miss. Both lines of the window are now read as code only. + +The fix is a prefix test and not block tracking, deliberately: a `/* … */` body +line that does not start with `*` still reads as code. There are 539 block +comments under `ui/` and not one such line, because every KDoc here stars its +body -- and three identical rules spelled three ways would be worse than one +limitation written down. diff --git a/docs/scripts/m3-touch-targets.py b/docs/scripts/m3-touch-targets.py index 56470da1..85f0bc07 100755 --- a/docs/scripts/m3-touch-targets.py +++ b/docs/scripts/m3-touch-targets.py @@ -14,6 +14,14 @@ a pointer on desktop has to land on. Components that carry their own minimum -- IconButton, Button, Checkbox, ListItem and the rest of material3 -- are not `.clickable` call sites and never appear here. +Comments are not code, in either direction. A KDoc line explaining that a row uses a +clickable `Card` *rather than* a `Modifier.clickable` was counted as a call site, which +breaks a zero budget and so breaks `./gradlew check`; and a comment above a real call site +that merely mentions `minimumInteractiveComponentSize` would have excused it. The first is +noise and the second is a miss, so both lines of the window are read as code only. This is +the same thing m3-audit.sh's `NOT_A_COMMENT` exists for, spelled as m3-title-case.py +spells it -- see the note there on a colour comment counting as a colour. + Usage: m3-touch-targets.py [--list] """ import os, re, sys @@ -21,6 +29,18 @@ import os, re, sys UI = 'composeApp/src/commonMain/kotlin/press/mantra/compose/ui' +def _is_comment(line): + """Whether `line` is a comment line: `//`, or a `/*` block's opener or body. + + A prefix test rather than block tracking, because that is what the other two checkers + do and three identical rules spelled three ways is worse than one limitation. The + limitation: a `/* ... */` body line that does not start with `*` reads as code. There + are 539 block comments under `ui/` and not one such line, since every KDoc here stars + its body -- if that ever stops being true, this is the function to make stateful. + """ + return line.lstrip().startswith(('//', '*', '/*')) + + def offenders(): found = [] for root, _, files in os.walk(UI): @@ -32,11 +52,14 @@ def offenders(): for i, line in enumerate(lines): if '.clickable' not in line: continue - if line.lstrip().startswith('//') or line.startswith('import '): + if _is_comment(line) or line.startswith('import '): continue # The modifier may sit on this line or on the one above, since a chain - # broken across lines is the common shape. - window = '\n'.join(lines[max(0, i - 1):i + 1]) + # broken across lines is the common shape. Comment lines are dropped from + # the window rather than kept: an exemption has to be in the code. + window = '\n'.join( + l for l in lines[max(0, i - 1):i + 1] if not _is_comment(l) + ) if 'minimumInteractiveComponentSize' in window: continue found.append((path, i + 1, line.strip()[:90]))