Sweep comments to the load-bearing minimum - #5
Merged
Merged
Conversation
One rule applied repo-wide: no comment unless the code would read as a bug without it, and then a single line. Rationale moves to architecture/ and planning/changes/, which is where this repo already keeps it. Authored comment lines go from 203 to 25 - app/ 127 to 19, tests/ 54 to 5, migrations/ 6 to 1. Most of what went duplicated architecture/*.md verbatim; the Transaction.__aexit__ hazard alone was written out three times in app/use_cases/ and once more in chats.md. Most test comments explained what the test proved, which the test name already did. Each block was checked against architecture/ before deletion. One gap turned up and is now documented rather than dropped: service_debug's echo/echo_pool log bound parameters including password_hash, which had no doc home and now sits in architecture/auth.md beside the capture_parameters=False note. # revision identifiers came from our own script.py.mako, so it is gone at the source as well as from the four existing migrations. Alembic's # ### autogenerate markers stay - the renderer reintroduces them every time. CLAUDE.md now states the rule and enumerates what may keep a comment.
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.
Applies one rule repo-wide: no comment unless the code would read as a bug
without it, and then a single line. Rationale moves to
architecture/andplanning/changes/, which is where this repo already keeps it.Full write-up in
planning/changes/2026-08-21.04-comment-sweep.md.Numbers
app/tests/migrations/(authored)Alembic's 16
# ###autogenerate markers stay — they come from the renderer,not our template, so deleting them restarts the fight on every migration.
# revision identifiers, used by Alembic.did come from ourmigrations/script.py.mako, so it is gone at the source as well as from thefour existing files.
What survives
The whole permitted category, and nothing else:
join_transaction_mode,populate_existing,capture_parameters=False,path_separatororm.foreign()on a column carrying noForeignKeyreturnfrom inside anasync with self.transaction:blockAuthenticateUserUseCasehashing apassword for an unknown username
What went, and where it went
Most of the deleted prose duplicated
architecture/*.mdverbatim — theTransaction.__aexit__hazard was written out three times inapp/use_cases/and once more in
chats.md. Most test comments explained what the test proved,which is the test name's job:
test_non_author_member_cannot_edit_messagecarried
# bob is a member of the chat but not the author.Every block was grepped against
architecture/before deletion. One gap turnedup and was filled rather than dropped:
service_debug'secho/echo_poollogbound parameters including
password_hashon every registration. That had nodoc home and now sits in
architecture/auth.mdbeside thecapture_parameters=Falsenote it parallels.CLAUDE.mdstates the rule and enumerates the permitted category, and its onepointer at a source comment (
orm.DeclarativeBase.metadata) now carries thereason inline instead.
Verification
just test— 109 passed, 100% coverage.just test-migrations— 4 passed.just lint,just check-planning— clean.The suites are the check that matters: this change removes no code, so green
means every deletion was in fact a comment.