Repository navigation
Move rationale essays out of source comments into docs/ and write the comment rule into CONTRIBUTING - #233
Merged
Conversation
Comment density had reached 261 comment lines against 462 total in Iban.kt and 120 against 360 in gradle.yml, most of it "why we did not do X" prose carrying issue numbers, repeated across declarations and duplicated in the README. The reasoning is worth keeping, so it moved rather than being deleted: * docs/209-api-design-notes.md - the public-surface decisions (String receivers, Iban.parse, the IBAN typealias, ASCII-only rejection, the sealed Kind, the Instant encoding). * docs/209-implementation-notes.md - the internal ones (eager pretty, the cached country index, validate's check order, the allocation-free fold, the ASCII helpers). * docs/209-build-script-notes.md - the Gradle configuration, including the linuxArm64Test emulator block, abiValidation and the consumer Kotlin floor. * docs/209-ci-workflow-notes.md - the workflows, the composite action, dependabot and the two registry scripts. What stays in the code is the KDoc contract plus a one-line pointer where the choice is not obvious from the signature. Issue numbers are gone from code comments; the doc pages carry them instead. CONTRIBUTING.md now states the rule so this does not accumulate again: KDoc states the contract, an inline comment explains one non-obvious line in one or two sentences, anything longer is a doc page. No API, behaviour or message change; the API dumps are untouched. Co-Authored-By: Claude <noreply@anthropic.com>
…ionale-docs # Conflicts: # CHANGELOG.md # kiban/src/commonMain/kotlin/nl/bijdorpstudio/kiban/CountryCodes.kt # kiban/src/commonMain/kotlin/nl/bijdorpstudio/kiban/Iban.kt
…ionale-docs # Conflicts: # .github/workflows/gradle.yml # kiban/build.gradle.kts
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.
What changed
Comment density was the problem #209 names: 261 comment lines against 462 total in
Iban.kt, 120 against 360 ingradle.yml, and the dominant pattern was "why we did not do X" prose carrying issue numbers — theStringreceiver rationale repeated on three declarations, 16 lines of it for one line oftypealias, a page each on the Dokka block and thelinuxArm64Testblock, and workflow history thatgit blamealready holds.The reasoning itself was worth keeping, so it moved rather than being deleted. Four new pages under
docs/:docs/209-api-design-notes.mdStringreceivers,Iban.parsenext toIban.invoke, the JVM-onlyIBANtypealias, ASCII-only reject-not-normalize, the sealedMalformed.Kind, the midnight-UTCInstantencoding, the defensiveknownCountryCodescopydocs/209-implementation-notes.mdpretty, the cached country index,ofValidatedvsinvoke,validate's check ordering,toPlain's fast path, the pre-sizedaddSpacesbuilder, the ASCII case-bit helpers,Modulo97's running remainderdocs/209-build-script-notes.mdexplicitApi,abiValidationand the duplicated android dump, versioned API docs,signPublications,verifyPublicationTargets, the wholelinuxArm64Testemulator block guard by guard, the consumer Kotlin floor, the TestBalloon pin and root-script declarationdocs/209-ci-workflow-notes.mdgradle.yml(concurrency, matrix shape, build scans, the JUnit reporting inputs, the two failure-diff steps),publish.yml(the tag guard, thereleaseenvironment, attestation staging, the docs archive),registry-sync.ymland the two registry scripts,ios-interop-verify.yml,scorecard.yml,cache-konan,dependabot.ymlWhat stays in the code is the KDoc contract plus a one-line pointer where the choice is not obvious from the signature. Issue numbers are gone from code comments — including the three outside the files #209 lists (
build.gradle.kts,IbanTest.kt,MalformedKindTest.kt); the doc pages carry them instead, as the existingdocs/write-ups already do.CONTRIBUTING.mdgains a Comments and documentation section stating the rule so this does not accumulate again: KDoc on a public declaration states the contract only, an inline comment explains one non-obvious line in one or two sentences, anything longer is a doc page, and issue numbers belong in the pull request andgit log.Against the issue's "done when": no source, build or workflow file now carries a comment block longer than roughly five lines that is not a KDoc contract description. What remains over five lines is the Apache licence header, KDoc contract blocks with their
@param/@returntags, the two registry scripts' usage headers (their--helptext, effectively), the generated/* AD */markers inCountryCodesData.kt, and three six-to-ten-line file-purpose headers that each end in a pointer.Out of scope and left alone: the README's duplicate of the
Stringreceiver rationale (#212 owns the README), thedocs/index and title-first file names (#214), and whetherlinuxArm64Testshould exist at all (#218) — its block is documented as it stands, with a note pointing at that question.Verification
Run locally on a Linux container, with an Android SDK installed per
CLAUDE.md:./gradlew jvmTest— passed. (Three retries were needed first: Maven Central returned HTTP 429 to the container, not a test failure.)./gradlew ktfmtCheck— passed, after./gradlew ktfmtFormatreflowed the trimmed KDoc../gradlew checkKotlinAbi— passed with the committed dumps untouched, which is the check that matters here: this is a comment-only change to the code, sokiban/api/jvm/kiban.api,kiban/api/android/kiban.apiandkiban.klib.apihad to come back byte-identical, and they did. (apiCheckis the standalone validator's task name and no longer exists in this build;checkKotlinAbireplaced it in Move off the standalone binary-compatibility-validator onto the Kotlin Gradle Plugin's built-in ABI validation #182.)dependabot.ymlandcache-konan/action.ymlre-parsed as YAML after editing.CONTRIBUTING.mdand the newCHANGELOG.mdentry resolves to a file that exists.No tests were added, and none should be: the change removes and relocates comments. It adds no behaviour, no declaration and no API; the existing suite passing unchanged, together with the unchanged ABI dumps, is what establishes that nothing but comments moved.
Not verified locally: the Apple-target tests, the XCFramework and
samples/swift-console(no Xcode on a Linux container), andjsTest/wasmJsTest(the sandbox proxy breaks Karma's dependency fetch). Neither is plausibly affected by a comment change, but CI's full matrix is the real check. The twoscripts/*.main.kts--self-checkruns thatregistry-sync.ymlperforms could not run either — thekotlinCLI is not installed here — so the edits there were kept strictly inside block comments.A
CHANGELOG.mdentry went into the Documentation group of the unreleased section.Closes #209
Generated by Claude Code