Skip to content

Remove deprecated WithBaseOption wrapper function (#57) - #102

Merged
harrydayexe merged 3 commits into
v3-changesfrom
57-remove-deprecated-withbaseoption-wrapper-function
Sep 18, 2026
Merged

harrydayexe merged 3 commits into
v3-changesfrom
57-remove-deprecated-withbaseoption-wrapper-function

Conversation

@harrydayexe

@harrydayexe harrydayexe commented Sep 18, 2026 •

Copy link
Copy Markdown
Owner

Closes #57

Note

This pull request was written by rain running Claude Code. It has not been reviewed by a human. CI and an automated review pass run before it is handed over.

It targets v3-changes rather than the default branch: 57-remove-deprecated-withbaseoption-wrapper-function was already linked to the issue, and its history says it was cut from v3-changes.

Issue

#57: Remove deprecated WithBaseOption wrapper function

What the agent reports

Pushed. Branch is two commits ahead of v3-changes.

What changed

78c3c5a — the removal (pre-existing on the branch, verified not re-done). Deletes func WithBaseOption(BaseOption) GeneratorOption from pkg/config/generatorOption.go and adds pkg/config/generatorOption_test.go covering the replacement BaseOption.AsGeneratorOption() — that it lifts logger, blog root and assets dir, and that it leaves the generator-specific func pointers nil so the else if application chain can't pick a field the caller never set.

204588f — doc follow-up I added. pkg/config/doc.go's "Option types" section said BaseOption flows into the specialised types "via the embedded BaseOption" but never named the methods that do the lifting. With the wrapper gone, As*Option() is the only route, so the package overview described a capability with no stated way to reach it. Added a short paragraph naming AsGeneratorOption / AsWatcherOption / AsServerOption with one example each. This is the CLAUDE.md rule that API changes carry doc updates; I kept it to the one surface the removal actually invalidated.

Verification

  • grep for WithBaseOption across all *.go and *.md: no matches anywhere in the repo, so no dangling call sites or stale docs.
  • Per CI (.github/workflows/test.yml runs vet + test + -race per module): go vet ./..., go test ./... and go test -race ./... all pass for both . and cli. go vet ./... passes for integration (its tests need Docker and were not run).
  • gofmt -l . clean.
  • go doc ./pkg/config renders the new section correctly.
  • New tests run and pass under -count=1 (not just cached).
  • addlicense -check could not run — the binary isn't installed in this environment. The new test file carries the MPL header matching its siblings, and doc.go was edited in place, so the license check should pass.

For the reviewer

Two things worth a look:

The doc examples are hand-verified, not compiled. I initially wrote server.New(cfg, config.WithLogger(logger).AsServerOption()), which is wrong — server.New takes (posts fs.FS, opts config.ServerConfig), not variadic options. I checked the real signatures and corrected it to construct a ServerConfig with a Server: []config.BaseServerOption{...} field. The generator.New and watcher.New examples match their actual signatures. Since these live in comments nothing enforces them, so a second read is worthwhile.

Scope judgement. The issue names only WithBaseOption. I did not touch the sibling As*Option methods or the AsOption() methods on the individual config types. The doc paragraph is the sole addition beyond the literal removal.

The commit message on 78c3c5a already carries a BREAKING CHANGE: footer, which matters if release tooling derives the v3.0.0 notes from commit trailers.

Changelog (#102)

📚 Documentation

  • (config) document As*Option lifting in package docs
  • (config) lift base options in godoc examples

❓ Uncategorised!

  • feat(config)!: remove deprecated WithBaseOption wrapper

harrydayexe and others added 3 commits September 18, 2026 15:10
WithBaseOption was deprecated in favour of BaseOption.AsGeneratorOption,
which all call sites already use. Remove it now that the public API can
break, and cover the replacement method directly in pkg/config.

BREAKING CHANGE: config.WithBaseOption has been removed. Call
BaseOption.AsGeneratorOption() on the base option instead, e.g.
config.WithLogger(logger).AsGeneratorOption().

Refs #57

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
With WithBaseOption gone, BaseOption.AsGeneratorOption and its siblings are
the only way to pass a BaseOption to a constructor. The package overview
described BaseOption flowing into the specialised option types but never
named the methods that do the lifting, so add them to the "Option types"
section with a worked example for each.

Refs #57

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The godoc examples for WithBlogRoot and WithLogger passed a BaseOption
straight into generator.New and watcher.New, which only accept
GeneratorOption and WatcherOption. With WithBaseOption removed, AsGeneratorOption
and AsWatcherOption are the only route, so the examples now show them and
compile as written.

Also correct the WithBlogRoot doc comment, which said it returns an
"Option" rather than a BaseOption.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@harrydayexe

Copy link
Copy Markdown
Owner Author

Automated review

I read the diff, the full pkg/config option files, the real constructor signatures (generator.New, watcher.New, server.New, ServerConfig), and ran the new tests and go vet locally.

The removal itself is correct and complete: no remaining reference to WithBaseOption anywhere in the repo (all file types, not just .go/.md), the replacement AsGeneratorOption / AsWatcherOption / AsServerOption all exist, the new doc paragraph's three examples match the actual signatures (including the ServerConfig{Server: []config.BaseServerOption{...}} form, which is the correct one for server.New(posts fs.FS, opts config.ServerConfig)), and the four new tests compile and pass. The tests are characterization tests for the surviving method rather than tests that fail without the change — that's the right shape for a pure deletion, since the compiler is what enforces the removal. No safety concerns; nothing beyond the issue's scope except the doc paragraph, which is justified by CLAUDE.md.

One finding:

  1. pkg/config/doc.go:164-168 (and the same bug at pkg/config/baseOption.go:97 and pkg/config/baseOption.go:117) — these godoc examples pass a BaseOption straight into a constructor that only accepts the specialised type, so they do not compile:

    • doc.go:165-168: generator.New(fsys, renderer, config.WithBlogRoot("/blog/")) — WithBlogRoot returns BaseOption, generator.New takes ...config.GeneratorOption.
    • baseOption.go:117: identical generator.New(fsys, renderer, config.WithBlogRoot("/blog/")).
    • baseOption.go:97: watcher.New("posts/", config.WithLogger(logger)) — watcher.New takes ...config.WatcherOption.

    These are pre-existing on v3-changes, not introduced here, but this PR is the reason they matter: commit 204588f adds a paragraph at doc.go:121-131 stating that "Option functions that return a BaseOption (WithLogger, WithBlogRoot, WithAssetsDir) cannot be passed to a constructor directly", and then ~35 lines below in the same rendered go doc ./pkg/config output the package shows a user doing exactly that. With WithBaseOption gone, As*Option() is the only route, so a reader copying doc.go:167 gets a type error with no alternative shown nearby. The doc commit fixed the omission it identified but left the contradicting example in the file it edited.

    Fix (comment text only, three lines): doc.go:167 → config.WithBlogRoot("/blog/").AsGeneratorOption(),; baseOption.go:117 → gen := generator.New(fsys, renderer, config.WithBlogRoot("/blog/").AsGeneratorOption()); baseOption.go:97 → w, err := watcher.New("posts/", config.WithLogger(logger).AsWatcherOption()).

Agent response

Pushed to 57-remove-deprecated-withbaseoption-wrapper-function as 345e01c.

Review points

1. Non-compiling godoc examples at pkg/config/doc.go:167, pkg/config/baseOption.go:97 and baseOption.go:117 — fixed.

The reviewer is correct on all three. I confirmed the signatures: generator.New(posts fs.FS, renderer *TemplateRenderer, opts ...config.GeneratorOption) (pkg/generator/generator.go:91) and watcher.New(path string, opts ...config.WatcherOption) (pkg/watcher/watcher.go:43), while WithBlogRoot and WithLogger both return BaseOption. The reviewer's point about why it matters here is also right: doc.go:122-131 now tells the reader base options can't be passed directly, and the old example 35 lines later did exactly that with no alternative in view.

Changes:

  • doc.go:167 → config.WithBlogRoot("/blog/").AsGeneratorOption(),
  • baseOption.go:117 → generator.New(fsys, renderer, config.WithBlogRoot("/blog/").AsGeneratorOption())
  • baseOption.go:97 → watcher.New("posts/", config.WithLogger(logger).AsWatcherOption())

Beyond the three lines, I swept the whole repo (all file types, not just .go/.md) for every WithLogger / WithBlogRoot / WithAssetsDir call site that isn't lifted through an As*Option. The only other unlifted hits are legitimate: pkg/server/handler.go:69, because server.Handler genuinely takes ...config.BaseOption; README.md:231 and the pkg/parser docs, which use parser's own unrelated Option type; and test files that construct BaseOption values deliberately. So these three were the complete set.

I also corrected one thing adjacent to the fix: WithBlogRoot's doc comment opened with "returns an Option", which names a type that doesn't exist in this package — it now says BaseOption, matching WithLogger and WithAssetsDir.

Verification

  • just vet (library, cli, integration) — clean.
  • just test (library + cli) — all packages pass.
  • I extracted the three corrected snippets into a throwaway module with a replace onto this worktree and built it, so the examples are verified to compile as written rather than just eyeballed.

Nothing declined.


Posted by rain. Both halves were written by Claude Code; treat them as a starting point for human review, not a substitute for it.

@harrydayexe harrydayexe linked an issue Sep 18, 2026 that may be closed by this pull request
@harrydayexe
harrydayexe marked this pull request as ready for review September 18, 2026 17:12
@harrydayexe
harrydayexe merged commit e60bf19 into v3-changes Sep 18, 2026
6 checks passed
@harrydayexe
harrydayexe deleted the 57-remove-deprecated-withbaseoption-wrapper-function branch September 18, 2026 17:13
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.

Remove deprecated WithBaseOption wrapper function

1 participant