Turn the project's rules into tests, and generate a map of where everything lives
The first question about any change is which file it belongs in, and the three documents that
kept being asked to answer it - README (the tour), ROADMAP (what is done) and CHANGELOG (why it is
like that) - were never going to. docs/CODE_MAP.md answers it: all 74 source files with the purpose
taken from the KDoc already sitting next to them, their size, and the tests that cover them,
followed by every test name in the suite. Those names are sentences about behaviour in this project
("a batch rename is a single undo step"), so the index doubles as the specification, and it is the
cheapest way for a reader, human or otherwise, to find out whether something is already covered.
The map is generated by tools/make-code-map.py rather than written, because a hand-maintained
description of 74 files is wrong within a milestone and nobody notices. The script harvests
declarations and comments; --check fails the moment the file stops matching the tree, and it caught
this very change (350 tests to 355); and the file says so about itself in its first line. A file is
described by the KDoc of the declaration that shares its name, which is the convention this
codebase already follows, and where no declaration matches, the first KDoc is used and the file is
listed at the end - so 15 fallbacks out of 74 are visible rather than assumed. Test files are
attached by the class they declare (NameTest -> Name.kt), which is a name-based match and therefore
approximate: the one honest weakness, and worth knowing before trusting a row that says a file has
no tests.
AGENTS.md is the one page that points at the map: what the app is, the two files to read before the
source, the exact commands, the layout with its dependency direction (ui -> domain -> data -> core,
with core free of Android), the rules that have tests behind them, what to read on demand and what
not to read at all, the definition of done, and the traps that have cost real time here - MIUI's two
install gates, assembleDebug racing an adb install, R8 stripping what only a resource names, and adb
input text not delivering a slash.
The same pass made four of the project's stated rules fail instead of merely being stated, in
ProjectInvariantsTest, which reads the source tree: core/ imports no Android (the engine stays
testable without Robolectric), no Text(...) in ui/ is handed a string literal (text comes from
strings.xml, which the pseudo-locale build exercises), the manifest requests exactly the six
permissions the app is built around and never INTERNET, and separator handling lives in core/ alone.
Three of the four were already true. The fourth found a bug: createFolder checked only that the name
was not empty, so "notes/final" was resolved by StoragePath into the notes folder that happened to
exist - the dialog offered one folder in the folder on screen and the app created a different one.
Rename refused the same input, because the rule has exactly one implementation and only one of its
two callers used it.
Both callers now go through isUsableName, and the shared name dialog says why a name is refused -
"A name cannot contain a slash", while it is typed, with its button disabled - rather than appearing
to do nothing when it is pressed. The batch dialog and both single-name dialogs read the same
string, so the rule reads the same wherever a name is entered. Verified on the phone: a slash typed
into New folder shows the note with Create the only disabled node in the dump, and deleting the
slash clears the note and re-enables it.
355 JVM tests (5 added: the four invariants and the folder-name refusal), lint clean, the map
current. The source-level rules state their own limits in their comments - the literal rule catches
a literal as the whole text of a Text(...) and not one concatenated into resource text, because
seeing that would take a Kotlin parser, and a rule that pretends otherwise is worse than one that
says where it stops.
This commit is contained in:
@@ -27,6 +27,7 @@ import de.softwareapp_hb.filemanager.core.BatchRename
|
||||
import de.softwareapp_hb.filemanager.core.BatchRenamePlan
|
||||
import de.softwareapp_hb.filemanager.core.ConflictPolicy
|
||||
import de.softwareapp_hb.filemanager.core.FileEntry
|
||||
import de.softwareapp_hb.filemanager.core.isUsableName
|
||||
|
||||
/**
|
||||
* Renaming several files at once, with the numbering worked out before anything moves.
|
||||
@@ -136,7 +137,7 @@ private const val PREVIEW_NAMES = 3
|
||||
@Composable
|
||||
private fun batchRenameNote(base: String, usable: Boolean, plan: BatchRenamePlan?): String = when {
|
||||
base.isEmpty() -> ""
|
||||
!usable -> stringResource(R.string.browser_rename_many_bad_name)
|
||||
!usable -> stringResource(R.string.browser_name_bad)
|
||||
|
||||
plan is BatchRenamePlan.Blocked ->
|
||||
stringResource(R.string.browser_rename_many_taken, plan.taken.first())
|
||||
@@ -165,6 +166,12 @@ fun NameDialog(
|
||||
onConfirm: (String) -> Unit,
|
||||
) {
|
||||
var value by remember { mutableStateOf(initialValue) }
|
||||
val cleaned = value.trim()
|
||||
// The same rule the batch dialog applies, because it is the same rule: a name is one path
|
||||
// segment. Saying it here is the difference between a dialog that refuses and one that
|
||||
// appears to do nothing — a name with a slash in it used to be accepted by the button and
|
||||
// then quietly dropped by the view model.
|
||||
val usable = isUsableName(cleaned)
|
||||
|
||||
AlertDialog(
|
||||
onDismissRequest = onDismiss,
|
||||
@@ -176,18 +183,27 @@ fun NameDialog(
|
||||
onValueChange = { value = it },
|
||||
modifier = Modifier.fillMaxWidth(),
|
||||
singleLine = true,
|
||||
isError = value.isNotBlank() && !usable,
|
||||
label = { Text(stringResource(R.string.browser_name_label)) },
|
||||
keyboardOptions = KeyboardOptions(
|
||||
capitalization = KeyboardCapitalization.None,
|
||||
imeAction = ImeAction.Done,
|
||||
),
|
||||
)
|
||||
if (value.isNotBlank() && !usable) {
|
||||
Spacer(Modifier.height(8.dp))
|
||||
Text(
|
||||
text = stringResource(R.string.browser_name_bad),
|
||||
style = MaterialTheme.typography.bodySmall,
|
||||
color = MaterialTheme.colorScheme.error,
|
||||
)
|
||||
}
|
||||
}
|
||||
},
|
||||
confirmButton = {
|
||||
TextButton(
|
||||
onClick = { onConfirm(value) },
|
||||
enabled = value.isNotBlank(),
|
||||
onClick = { onConfirm(cleaned) },
|
||||
enabled = cleaned.isNotBlank() && usable,
|
||||
) {
|
||||
Text(confirmLabel)
|
||||
}
|
||||
|
||||
@@ -1632,7 +1632,10 @@ class BrowserViewModel(
|
||||
val target = _state.value.path ?: return
|
||||
val source = storage.activeSource() ?: return
|
||||
val cleanName = name.trim()
|
||||
if (cleanName.isEmpty()) return
|
||||
// The same rule a rename needs, for the same reason: a name is one path segment, so
|
||||
// `notes/final` must be refused rather than resolved into the `notes` folder that happens
|
||||
// to be sitting there — the dialog offered one new folder, not a folder inside a subfolder.
|
||||
if (!isUsableName(cleanName)) return
|
||||
|
||||
viewModelScope.launch {
|
||||
val created = runCatching { source.createDirectory(target.child(cleanName)) }.getOrDefault(false)
|
||||
|
||||
@@ -159,7 +159,7 @@
|
||||
<!-- %1$s is a name the pattern asks for that something in this folder already holds. -->
|
||||
<string name="browser_rename_many_taken">%1$s is already in this folder</string>
|
||||
<string name="browser_rename_many_nothing">Every selected file already has that name</string>
|
||||
<string name="browser_rename_many_bad_name">A name cannot contain a slash</string>
|
||||
<string name="browser_name_bad">A name cannot contain a slash</string>
|
||||
<plurals name="message_renamed_many">
|
||||
<item quantity="one">Renamed %1$d file</item>
|
||||
<item quantity="other">Renamed %1$d files</item>
|
||||
|
||||
@@ -0,0 +1,157 @@
|
||||
package de.softwareapp_hb.filemanager
|
||||
|
||||
import java.io.File
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
|
||||
/**
|
||||
* The rules this project states in prose, checked against the source instead of trusted.
|
||||
*
|
||||
* Each of them is a sentence that already appears in the README, the CHANGELOG or a KDoc, and each
|
||||
* is the kind of claim that stays true for a year and then quietly stops being true — a stray
|
||||
* `import android.` in `core/`, a hardcoded label in a dialog, a permission added for something
|
||||
* that seemed to need it. The failure message is meant to be the documentation: it says which rule
|
||||
* broke, why the rule exists and what to do instead.
|
||||
*
|
||||
* These are source-level tests rather than behaviour, so they read the tree and need a working
|
||||
* directory inside the repository. Gradle's `Test` task provides one; if it ever does not, that is
|
||||
* reported as a failure rather than passed silently, because an invariant test that cannot see the
|
||||
* source is worse than no test at all.
|
||||
*/
|
||||
class ProjectInvariantsTest {
|
||||
|
||||
@Test
|
||||
fun `core is pure Kotlin, with no Android imports`() {
|
||||
val offenders = kotlinFiles(mainDir("core"))
|
||||
.flatMap { file -> imports(file).map { file.name to it } }
|
||||
.filter { (_, line) ->
|
||||
line.startsWith("import android.") || line.startsWith("import androidx.")
|
||||
}
|
||||
assertTrue(
|
||||
"core/ is the engine the JVM tests reach without a framework, and the README says so " +
|
||||
"in as many words. An Android import here means the logic it holds can no longer " +
|
||||
"be tested without Robolectric, so either move the platform call up into data/ or " +
|
||||
"ui/, or give core/ a plain-Kotlin seam. Found: " +
|
||||
offenders.joinToString { (name, line) -> "$name -> $line" },
|
||||
offenders.isEmpty(),
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `user-visible text comes from string resources, not from literals`() {
|
||||
// Deliberately narrow: this catches a literal as the *whole* text of a `Text(...)`, which
|
||||
// is the way a hardcoded label usually arrives. It does not catch a literal concatenated
|
||||
// into resource text (" · " between two formatted halves), because seeing that would take
|
||||
// a Kotlin parser rather than a line scan — and a rule that pretends otherwise would be
|
||||
// worse than one that states its own limit.
|
||||
val literal = Regex("\\bText\\(\\s*\"")
|
||||
val offenders = kotlinFiles(mainDir("ui"))
|
||||
.flatMap { file -> codeLines(file).map { file.name to it.trim() } }
|
||||
.filter { (_, line) -> literal.containsMatchIn(line) }
|
||||
assertTrue(
|
||||
"Every string a user reads has to come from strings.xml: it is what the pseudo-locale " +
|
||||
"build (values-en-XA) exercises, and it is what the localisation section of the " +
|
||||
"README promises. Use stringResource(R.string.…) instead. Found: " +
|
||||
offenders.joinToString { (name, line) -> "$name -> $line" },
|
||||
offenders.isEmpty(),
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `the app asks for no permission that could reach the network`() {
|
||||
val manifest = repositoryRoot().resolve("app/src/main/AndroidManifest.xml").readText()
|
||||
val declared = Regex("<uses-permission\\s+android:name=\"([^\"]+)\"")
|
||||
.findAll(manifest)
|
||||
.map { it.groupValues[1] }
|
||||
.toSet()
|
||||
|
||||
assertFalse(
|
||||
"No INTERNET is the app's whole privacy claim — \"nothing leaves the device\" is only " +
|
||||
"true because the shipped APK cannot open a socket. This is the cheap version of " +
|
||||
"the aapt2 audit in the release checklist, and it exists because a dependency " +
|
||||
"pulling INTERNET in transitively would otherwise stay invisible until someone " +
|
||||
"ran that audit. Declared: $declared",
|
||||
declared.any { it.endsWith("INTERNET") },
|
||||
)
|
||||
assertEquals(
|
||||
"A permission is a decision, not a side effect. Everything here is deliberate — " +
|
||||
"all-files access with its Play declaration form, the pre-13 read and the pre-11 " +
|
||||
"write, the two foreground-service types, and notifications so progress can be " +
|
||||
"shown — so a new one means deciding it, writing down why in the manifest comment " +
|
||||
"and the README, and then updating this list on purpose.",
|
||||
setOf(
|
||||
"android.permission.MANAGE_EXTERNAL_STORAGE",
|
||||
"android.permission.READ_EXTERNAL_STORAGE",
|
||||
"android.permission.WRITE_EXTERNAL_STORAGE",
|
||||
"android.permission.FOREGROUND_SERVICE",
|
||||
"android.permission.FOREGROUND_SERVICE_DATA_SYNC",
|
||||
"android.permission.POST_NOTIFICATIONS",
|
||||
),
|
||||
declared,
|
||||
)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a name is one path segment, and that rule lives in one place`() {
|
||||
// `core/FileNames.isUsableName` is the rule; `core/StoragePath` and `core/ArchivePath` are
|
||||
// where separators are actually parsed. Anywhere else, a separator check is a second
|
||||
// opinion about what a name is — which is exactly how "New folder" came to create a folder
|
||||
// inside a subfolder while Rename refused the same input.
|
||||
val separator = Regex("(contains|indexOf|split|endsWith)\\(['\"]/['\"]\\)")
|
||||
val offenders = (mainDir().listFiles() ?: emptyArray())
|
||||
.filter { it.isDirectory && it.name != "core" }
|
||||
.flatMap { dir -> kotlinFiles(dir) }
|
||||
.flatMap { file -> codeLines(file).map { file.name to it.trim() } }
|
||||
.filter { (_, line) -> separator.containsMatchIn(line) }
|
||||
assertTrue(
|
||||
"A name that contains a separator is not a name: `StoragePath.child(\"a/b\")` resolves " +
|
||||
"into a subfolder, so an unguarded call renames or creates something other than " +
|
||||
"what the dialog offered. Ask core/FileNames.isUsableName instead of inspecting " +
|
||||
"the string here. Found: " +
|
||||
offenders.joinToString { (name, line) -> "$name -> $line" },
|
||||
offenders.isEmpty(),
|
||||
)
|
||||
}
|
||||
|
||||
// ------------------------------------------------------------------ reading the tree
|
||||
|
||||
private fun repositoryRoot(): File {
|
||||
var candidate = File("").absoluteFile
|
||||
while (true) {
|
||||
if (File(candidate, "settings.gradle.kts").isFile) return candidate
|
||||
candidate = candidate.parentFile
|
||||
?: throw AssertionError(
|
||||
"Could not find settings.gradle.kts above ${File("").absolutePath}. These " +
|
||||
"tests read the source tree, so the working directory has to be inside " +
|
||||
"the repository — Gradle's Test task normally sets one.",
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
private fun mainDir(vararg nested: String): File {
|
||||
var dir = repositoryRoot().resolve("app/src/main/java/de/softwareapp_hb/filemanager")
|
||||
for (part in nested) dir = dir.resolve(part)
|
||||
return dir
|
||||
}
|
||||
|
||||
private fun kotlinFiles(dir: File): List<File> =
|
||||
(dir.listFiles() ?: emptyArray()).sorted().flatMap { child ->
|
||||
when {
|
||||
child.isDirectory -> kotlinFiles(child)
|
||||
child.extension == "kt" -> listOf(child)
|
||||
else -> emptyList()
|
||||
}
|
||||
}
|
||||
|
||||
/** The lines of [file] that are code: a rule stated in a comment is documentation, not a bug. */
|
||||
private fun codeLines(file: File): List<String> =
|
||||
file.readLines().filterNot { line ->
|
||||
val trimmed = line.trimStart()
|
||||
trimmed.startsWith("//") || trimmed.startsWith("*") || trimmed.startsWith("/*")
|
||||
}
|
||||
|
||||
private fun imports(file: File): List<String> =
|
||||
codeLines(file).map { it.trim() }.filter { it.startsWith("import ") }
|
||||
}
|
||||
@@ -939,6 +939,21 @@ class BrowserViewModelTest {
|
||||
assertTrue(model.state.value.entries.any { it.name == "New folder" })
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `createFolder refuses a name that is a path`() {
|
||||
// `notes/final` resolves to `notes/final`, so an unguarded create would make a folder
|
||||
// *inside* `notes` — while the dialog offered one folder in the folder on screen.
|
||||
val model = viewModel()
|
||||
model.openDirectory(downloads)
|
||||
storage.addDirectory(downloads.child("notes"))
|
||||
|
||||
model.createFolder("notes/final")
|
||||
|
||||
assertFalse(storage.contains(downloads.child("notes").child("final")))
|
||||
assertNull("the refusal is the dialog's to explain, not a message after the fact",
|
||||
model.state.value.message)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `rename moves the entry to the new name`() {
|
||||
val model = viewModel()
|
||||
|
||||
Reference in New Issue
Block a user