Skip to content

refactor: remove explanatory comments in favour of self-explanatory code - #6

Merged
marsvogel merged 1 commit into
mainfrom
chore/remove-code-comments
Jul 24, 2026
Merged

refactor: remove explanatory comments in favour of self-explanatory code#6
marsvogel merged 1 commit into
mainfrom
chore/remove-code-comments

Conversation

@marsvogel

Copy link
Copy Markdown
Owner

Follow-up to #5, which recorded the rule. This applies it to everything that was already in the repository: 43 comment lines in Swift, 27 in the workflows, plus Dependabot config, CODEOWNERS and the build-number script.

Approach

A comment was never just deleted — its content moved into a name, a structure, or a test name:

Was a comment Is now
// Sunday = 1, Saturday = 7 enum Weekday: Int { case sunday = 1, monday, … }
for _ in 0..<7 for _ in 0..<daysPerWeek
// Hide from dock, // Create popover with calendar view, … hideFromDock(), makeCalendarPopover(), makeStatusItem(), makeFallbackRefreshTimer(), observeDateChanges()
// Right click - show context menu / // Left click - toggle popover showContextMenu() / togglePopoverVisibility(_:)
// Month navigation header, // Calendar grid, … extracted subviews monthNavigationHeader, weekdayHeaderRow, calendarGrid, todayButton
index >= 5, .frame(width: 28) firstWeekendSymbolIndex, weekNumberColumnWidth, dayColumnWidth
// 1 Jan 2026 is a Thursday, so it belongs to week 1. testJanuaryFirstBelongsToWeekOneWhenItFallsOnAThursday()
/// An ISO-8601 calendar pinned to UTC … utcISOCalendar

The five test methods became 14, one assertion group each, so a failure names the rule that broke.

One behavioural detail worth flagging: applicationDidFinishLaunching now builds the popover before the status item, so the button action can never reference a nil popover. The old order was safe only because everything runs synchronously on the main thread.

Kept deliberately

Comments a tool acts on: the # vX.Y.Z markers Dependabot parses on SHA-pinned actions, // swiftlint: directives, // MARK: navigation, and the .gitignore section headings. If you want the MARK lines and .gitignore headings gone too, say so and I will strip them.

Rationale that left the workflow files

Recorded here rather than in the YAML, per the rule:

  • fetch-depth: 0 — the "Set build number" build phase derives CFBundleVersion from the commit count, so CI needs the full history. Now also in CONTRIBUTING.md.
  • SwiftLint pinned to 0.65.0 and checksum-verified — a new lint release must not break --strict CI overnight, and a swapped release asset must not run unnoticed.
  • CODE_SIGN_IDENTITY="-" — ad-hoc signing keeps the app launchable on Apple Silicon without a paid Developer ID.
  • ditto instead of zip — preserves the .app bundle structure and its ad-hoc signature.
  • permissions: {} at workflow level — deny-all by default, each job opts into its minimum. The release job is separate so write scopes exist only where the release is created.
  • The release step checks for an existing tag so a re-run on the same commit does not hard-fail.
  • CodeQL runs on PRs against main only (other base branches have no baseline to compare against), skips Markdown-only PRs, and cancels superseded runs to free macOS runner slots.
  • Scripts/increment-build-number.sh is a reference copy — the script that actually runs is inlined into the Xcode build phase, because the build sandbox cannot read external files. Now in CONTRIBUTING.md.

Verification

  • swiftlint lint --strict — 0 violations across 5 files (same 0.65.0 as CI).
  • xcodebuild test — 14 tests, 0 failures.
  • xcodebuild -target QuickWeek -configuration Release build — succeeds.
  • Launched the built binary as a separate process: starts clean, no crash in applicationDidFinishLaunching.
  • actionlint and shellcheck pass.

Every explanatory comment is gone from Swift sources, workflows, Dependabot
config, CODEOWNERS and the build-number script — doc comments included. What a
comment used to say is now carried by a name, a structure, or a test name:

- WeekCalculator: `Weekday` enum replaces the "Sunday = 1, Saturday = 7"
  note, `daysPerWeek` replaces the bare 7.
- AppDelegate: `applicationDidFinishLaunching` is split into `hideFromDock`,
  `makeStatusItem`, `makeFallbackRefreshTimer`, `observeDateChanges`,
  `makeCalendarPopover`; the click handler into `showContextMenu` and
  `togglePopoverVisibility`. The popover is now built before the status item so
  the button action can never reference a nil popover.
- CalendarView: the section comments become extracted subviews
  (`monthNavigationHeader`, `weekdayHeaderRow`, `calendarGrid`, `todayButton`),
  and the magic 28/32/5 become `weekNumberColumnWidth`, `dayColumnWidth`,
  `firstWeekendSymbolIndex`.
- Tests: the per-assertion comments become 14 named test cases, and the
  UTC-pinned calendar is named `utcISOCalendar`.

Knowledge that has no home in code moves to CONTRIBUTING.md: why CI needs the
full history, and that Scripts/increment-build-number.sh is a reference copy of
a build phase inlined in project.pbxproj. The remaining rationale is recorded
in the pull request.

Comments a tool acts on stay: SHA-pin version markers, swiftlint directives,
MARK navigation, .gitignore section headings.

Behaviour is unchanged: swiftlint --strict clean, 14 tests pass, Release build
succeeds and the app launches.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@marsvogel
marsvogel merged commit 2f5bfcd into main Jul 24, 2026
5 checks passed
@marsvogel
marsvogel deleted the chore/remove-code-comments branch July 24, 2026 19:19
marsvogel added a commit that referenced this pull request Jul 24, 2026
Merging #5 and #6 twenty-eight seconds apart put two Build runs on main at
once, and the older one's release step failed with HTTP 403 on POST /releases
while the newer one created v1.0.10 without trouble. The 403 was never
explained — the job holds contents:write, the protect-main ruleset targets
branches rather than tags, and the same configuration succeeded moments later —
but two release jobs racing to create refs in one repository is not a state
worth keeping either way.

The concurrency group queues runs per ref. On main nothing is cancelled, so
every merge still produces its release; on pull requests superseded runs are
cancelled as before, since a stale PR build has nothing to publish.

Known trade-off: three merges in quick succession would leave the middle run
queued, and GitHub drops a queued run when a newer one arrives — that commit
would get no release. For a repository that merges a few times a month that is
a better failure mode than a race.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant