From 32d864724ac7766d845ecf5b674a35a0db7b5988 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 23 Aug 2026 22:16:04 +0200 Subject: [PATCH 1/5] refact(options): chain options by value in formatting, genapp and gentesting Every option type took a pointer and returned nothing, so the options struct escaped to the heap and each package read a little differently from the next. They now chain by value, as go-openapi/core does: type ( Option func(options) options options struct{ ... } ) func WithX(v T) Option { return func(o options) options { o.x = v return o } } applyWithDefaults folds the chain left to right. Where a package declares more than one option type, the helper carries it: applyModWithDefaults, applyTidyWithDefaults. Those two keep returning an error, which comes from validating the assembled options rather than from any single option. A default that is not a zero value stays seeded inside the helper. Nothing declares an empty defaultOptions to match a package that has real defaults. WithResolvedImports and WithBuildFlags write into a map and a slice the copy shares, which stays correct because each chain starts from its own zero value. An option that appended to a seeded slice would not: two chains would write into one backing array, and into the seed itself. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- formatting/format.go | 2 +- formatting/options.go | 60 +++++++++++++++++++++++------------ formatting/resolve/resolve.go | 49 +++++++++++++++++++--------- genapp/genapp.go | 2 +- genapp/module.go | 2 +- genapp/module_options.go | 46 +++++++++++++++++---------- genapp/options.go | 54 ++++++++++++++++++++----------- genapp/tidy.go | 2 +- genapp/tidy_options.go | 58 +++++++++++++++++++++------------ gentesting/testutils.go | 31 ++++++++++++++---- 10 files changed, 202 insertions(+), 104 deletions(-) diff --git a/formatting/format.go b/formatting/format.go index 71a4b7a..bc70671 100644 --- a/formatting/format.go +++ b/formatting/format.go @@ -87,7 +87,7 @@ func sourceBytes[T Source](src T) []byte { } func format(w io.Writer, src []byte, opts ...Option) (*ImportsReport, error) { - o := optionsWithDefaults(opts) + o := applyWithDefaults(opts) var extraRules rules.Func if o.goFumpt { diff --git a/formatting/options.go b/formatting/options.go index daadc23..af0a35e 100644 --- a/formatting/options.go +++ b/formatting/options.go @@ -3,29 +3,37 @@ package formatting -// Option configures [Format]. -type Option func(*options) - -type options struct { - groups []string - goFumpt bool - forcePruning bool - simplifyAliases bool - resolved map[string]string -} +import "maps" + +type ( + // Option configures [Format]. + Option func(options) options + + options struct { + groups []string + resolved map[string]string + + goFumpt bool + forcePruning bool + simplifyAliases bool + } +) // WithImportGroups adds one import group per prefix, between the standard library and the rest. // // An import belongs to the first prefix it starts with, so pass the more specific prefix first. // Without this option the output has two groups: the standard library, then everything else. func WithImportGroups(prefixes ...string) Option { - return func(o *options) { + return func(o options) options { for _, prefix := range prefixes { if prefix == "" { continue } + o.groups = append(o.groups, prefix) } + + return o } } @@ -34,8 +42,10 @@ func WithImportGroups(prefixes ...string) Option { // Blank-import github.com/go-openapi/codegen/formatting/enable/gofumpt to make the rules available. // Without it [Format] returns [ErrNoGoFumpt] rather than printing without them. func WithGoFumpt() Option { - return func(o *options) { + return func(o options) options { o.goFumpt = true + + return o } } @@ -60,8 +70,10 @@ func WithGoFumpt() Option { // }), // ) func WithForceImportsPruning() Option { - return func(o *options) { + return func(o options) options { o.forcePruning = true + + return o } } @@ -80,18 +92,18 @@ func WithForceImportsPruning() Option { // github.com/go-openapi/codegen/formatting/resolve, which answers from the packages themselves rather // than from the machine, so one map serves every build. func WithResolvedImports(names map[string]string) Option { - return func(o *options) { + return func(o options) options { if len(names) == 0 { - return + return o } if o.resolved == nil { o.resolved = make(map[string]string, len(names)) } - for importPath, name := range names { - o.resolved[importPath] = name - } + maps.Copy(o.resolved, names) + + return o } } @@ -112,16 +124,22 @@ func WithResolvedImports(names map[string]string) Option { // // Nothing is dropped on a guess. Without evidence from the table or the map, every alias stays. func WithSimplifiedImportAliases() Option { - return func(o *options) { + return func(o options) options { o.simplifyAliases = true + + return o } } -func optionsWithDefaults(opts []Option) options { +// applyWithDefaults folds the chain over the zero options, left to right. +// +// The zero value is the default throughout: no group past the standard library and the rest, no +// gofumpt, no forced pruning, and no name the caller supplied. +func applyWithDefaults(opts []Option) options { var o options for _, apply := range opts { - apply(&o) + o = apply(o) } return o diff --git a/formatting/resolve/resolve.go b/formatting/resolve/resolve.go index 4fa1408..5710d54 100644 --- a/formatting/resolve/resolve.go +++ b/formatting/resolve/resolve.go @@ -26,14 +26,16 @@ const ( ErrUnresolved Error = "some import paths did not resolve" ) -// Option configures [Names]. -type Option func(*options) - -type options struct { - dir string - env []string - buildFlags []string -} +type ( + // Option configures [Names]. + Option func(options) options + + options struct { + dir string + env []string + buildFlags []string + } +) // WithDir loads the packages as if from dir. // @@ -41,8 +43,10 @@ type options struct { // it the current working directory is used, which is right only when the process already runs inside // that module. func WithDir(dir string) Option { - return func(o *options) { + return func(o options) options { o.dir = dir + + return o } } @@ -50,15 +54,19 @@ func WithDir(dir string) Option { // // Use it to pin GOFLAGS, GOPATH or GOMODCACHE. An empty slice leaves the process environment alone. func WithEnv(env []string) Option { - return func(o *options) { + return func(o options) options { o.env = slices.Clone(env) + + return o } } // WithBuildFlags passes flags to "go list", as in -tags or -mod=mod. func WithBuildFlags(flags ...string) Option { - return func(o *options) { + return func(o options) options { o.buildFlags = append(o.buildFlags, flags...) + + return o } } @@ -85,10 +93,7 @@ func Names(ctx context.Context, paths []string, opts ...Option) (map[string]stri return map[string]string{}, nil } - var o options - for _, apply := range opts { - apply(&o) - } + o := applyWithDefaults(opts) loaded, err := packages.Load(&packages.Config{ Context: ctx, @@ -172,3 +177,17 @@ func missingFrom(wanted []string, names, reasons map[string]string) []string { func oneLine(message string) string { return strings.Join(strings.Fields(message), " ") } + +// applyWithDefaults folds the chain over the zero options, left to right. +// +// The zero value is the default: go list runs in the working directory, with the process environment +// and no build flags. +func applyWithDefaults(opts []Option) options { + var o options + + for _, apply := range opts { + o = apply(o) + } + + return o +} diff --git a/genapp/genapp.go b/genapp/genapp.go index 90db06b..4dafa46 100644 --- a/genapp/genapp.go +++ b/genapp/genapp.go @@ -37,7 +37,7 @@ type GoGenApp struct { // [github.com/go-openapi/codegen/templates-repo.New], and a repository that would not build has // already reported why by the time it reaches here. func New(opts ...Option) (*GoGenApp, error) { - o := optionsWithDefaults(opts) + o := applyWithDefaults(opts) if o.templates == nil { return nil, fmt.Errorf("a templates repository is required, see WithTemplates: %w", ErrGenApp) diff --git a/genapp/module.go b/genapp/module.go index 059094b..cdd7332 100644 --- a/genapp/module.go +++ b/genapp/module.go @@ -30,7 +30,7 @@ const goModFile = "go.mod" // A go.mod already in the output path is left alone and reported as [fs.ErrExist], unless // [WithReplaceExisting] says otherwise. func (g *GoGenApp) InitModule(opts ...ModOption) error { - o, err := modOptionsWithDefaults(opts) + o, err := applyModWithDefaults(opts) if err != nil { return err } diff --git a/genapp/module_options.go b/genapp/module_options.go index 14c382c..ca27a71 100644 --- a/genapp/module_options.go +++ b/genapp/module_options.go @@ -15,16 +15,18 @@ import ( "golang.org/x/mod/module" ) -// ModOption configures the go.mod [GoGenApp.InitModule] writes. -type ModOption func(*modOptions) - -type modOptions struct { - modulePath string - goVersion string - toolchain string - requires []requirement - replace bool -} +type ( + // ModOption configures the go.mod [GoGenApp.InitModule] writes. + ModOption func(modOptions) modOptions + + modOptions struct { + modulePath string + goVersion string + toolchain string + requires []requirement + replace bool + } +) type requirement struct { path string @@ -37,8 +39,10 @@ type requirement struct { // The path is cleaned and slash-separated, and it is checked the way the go command checks it, so a // path no module could have is reported here rather than by the first build. func WithModulePath(pth string) ModOption { - return func(o *modOptions) { + return func(o modOptions) modOptions { o.modulePath = path.Clean(filepath.ToSlash(pth)) + + return o } } @@ -46,8 +50,10 @@ func WithModulePath(pth string) ModOption { // // It defaults to the version of Go this program was built with, which is what "go mod init" writes. func WithGoVersion(version string) ModOption { - return func(o *modOptions) { + return func(o modOptions) modOptions { o.goVersion = version + + return o } } @@ -61,12 +67,14 @@ func WithGoVersion(version string) ModOption { // needs a toolchain newer than the one installed. Set it to say which toolchain a generated module // is meant to build with, whatever is on the machine that generated it. func WithToolchain(name string) ModOption { - return func(o *modOptions) { + return func(o modOptions) modOptions { if name != "" && name != toolchainDefault && !strings.HasPrefix(name, "go") { name = "go" + name } o.toolchain = name + + return o } } @@ -76,8 +84,10 @@ func WithToolchain(name string) ModOption { // versions to start from rather than resolving every import from scratch. Mark a requirement // indirect when nothing the module itself holds imports it. func WithRequire(pth, version string, indirect bool) ModOption { - return func(o *modOptions) { + return func(o modOptions) modOptions { o.requires = append(o.requires, requirement{path: pth, version: version, indirect: indirect}) + + return o } } @@ -86,8 +96,10 @@ func WithRequire(pth, version string, indirect bool) ModOption { // Without it [GoGenApp.InitModule] leaves an existing file alone and reports [fs.ErrExist], as // "go mod init" does. func WithReplaceExisting(replace bool) ModOption { - return func(o *modOptions) { + return func(o modOptions) modOptions { o.replace = replace + + return o } } @@ -111,11 +123,11 @@ func defaultGoVersion() string { return fallback } -func modOptionsWithDefaults(opts []ModOption) (modOptions, error) { +func applyModWithDefaults(opts []ModOption) (modOptions, error) { o := modOptions{goVersion: defaultGoVersion()} for _, apply := range opts { - apply(&o) + o = apply(o) } if o.modulePath == "" || o.modulePath == "." { diff --git a/genapp/options.go b/genapp/options.go index 439ba23..1c5fe98 100644 --- a/genapp/options.go +++ b/genapp/options.go @@ -11,17 +11,19 @@ import ( repo "github.com/go-openapi/codegen/templates-repo" ) -// Option configures a [GoGenApp]. -type Option func(*options) - -type options struct { - templates *repo.Repository - outputPath string - formatOptions []formatting.Option - importsReporter func(string, *formatting.ImportsReport) - skipFormat bool - skipFormatFunc func(target string) bool -} +type ( + // Option configures a [GoGenApp]. + Option func(options) options + + options struct { + templates *repo.Repository + outputPath string + formatOptions []formatting.Option + importsReporter func(string, *formatting.ImportsReport) + skipFormat bool + skipFormatFunc func(target string) bool + } +) // WithTemplates sets the repository to render from. [New] needs it. // @@ -38,15 +40,19 @@ type options struct { // // app, err := genapp.New(genapp.WithTemplates(templates)) func WithTemplates(templates *repo.Repository) Option { - return func(o *options) { + return func(o options) options { o.templates = templates + + return o } } // WithOutputPath sets where [GoGenApp.RenderFile] writes. Targets are relative to that directory. func WithOutputPath(path string) Option { - return func(o *options) { + return func(o options) options { o.outputPath = path + + return o } } @@ -59,8 +65,10 @@ func WithOutputPath(path string) Option { // formatting.WithImportGroups("github.com/go-openapi", baseImport), // ) func WithFormatOptions(opts ...formatting.Option) Option { - return func(o *options) { + return func(o options) options { o.formatOptions = append(o.formatOptions, opts...) + + return o } } @@ -79,8 +87,10 @@ func WithFormatOptions(opts ...formatting.Option) Option { // Resolve the paths it lists once, then pass the names through // [formatting.WithResolvedImports] with [WithFormatOptions]. func WithImportsReporter(report func(template string, report *formatting.ImportsReport)) Option { - return func(o *options) { + return func(o options) options { o.importsReporter = report + + return o } } @@ -90,8 +100,10 @@ func WithImportsReporter(report func(template string, report *formatting.Imports // written, and finding out why means reading the output. Turn this on and the file lands // unformatted. func WithSkipFormat(skipped bool) Option { - return func(o *options) { + return func(o options) options { o.skipFormat = skipped + + return o } } @@ -99,8 +111,10 @@ func WithSkipFormat(skipped bool) Option { // // The default formats a target whose name ends in ".go" and copies anything else through. func WithSkipFormatFunc(skip func(target string) bool) Option { - return func(o *options) { + return func(o options) options { o.skipFormatFunc = skip + + return o } } @@ -109,11 +123,13 @@ func (o options) skipsFormat(target string) bool { return o.skipFormat || o.skipFormatFunc(target) } -func optionsWithDefaults(opts []Option) options { +// applyWithDefaults folds the chain over the zero options, left to right, then settles the one +// default that is not a zero value. +func applyWithDefaults(opts []Option) options { var o options for _, apply := range opts { - apply(&o) + o = apply(o) } if o.skipFormatFunc == nil { diff --git a/genapp/tidy.go b/genapp/tidy.go index 8796feb..8dd010b 100644 --- a/genapp/tidy.go +++ b/genapp/tidy.go @@ -34,7 +34,7 @@ const maxTidyOutput = 4096 // // What the command wrote is reported when it fails; pass [WithTidyOutput] to watch it as it runs. func (g *GoGenApp) TidyModule(ctx context.Context, opts ...TidyOption) error { - o, err := tidyOptionsWithDefaults(opts) + o, err := applyTidyWithDefaults(opts) if err != nil { return err } diff --git a/genapp/tidy_options.go b/genapp/tidy_options.go index 7e5c616..19ca540 100644 --- a/genapp/tidy_options.go +++ b/genapp/tidy_options.go @@ -11,43 +11,51 @@ import ( "golang.org/x/mod/modfile" ) -// TidyOption configures [GoGenApp.TidyModule]. -type TidyOption func(*tidyOptions) - -type tidyOptions struct { - goCommand string - goVersion string - compat string - output io.Writer - env []string - workspace bool - waitDelay time.Duration -} +type ( + // TidyOption configures [GoGenApp.TidyModule]. + TidyOption func(tidyOptions) tidyOptions + + tidyOptions struct { + goCommand string + goVersion string + compat string + output io.Writer + env []string + workspace bool + waitDelay time.Duration + } +) // WithGoCommand names the go binary to run. It defaults to "go", found on PATH. // // Pass an absolute path to run a toolchain the PATH does not point at. func WithGoCommand(command string) TidyOption { - return func(o *tidyOptions) { + return func(o tidyOptions) tidyOptions { if command != "" { o.goCommand = command } + + return o } } // WithTidyGoVersion passes -go to the command, as in "1.25.0", which sets the go directive while // tidying. func WithTidyGoVersion(version string) TidyOption { - return func(o *tidyOptions) { + return func(o tidyOptions) tidyOptions { o.goVersion = version + + return o } } // WithTidyCompat passes -compat to the command, as in "1.24", which keeps the checksums an older // go needs to load the module. func WithTidyCompat(version string) TidyOption { - return func(o *tidyOptions) { + return func(o tidyOptions) tidyOptions { o.compat = version + + return o } } @@ -56,8 +64,10 @@ func WithTidyCompat(version string) TidyOption { // The output is kept either way and reported when the command fails. Pass a writer to watch a tidy // that takes a while, since it downloads what the module requires. func WithTidyOutput(w io.Writer) TidyOption { - return func(o *tidyOptions) { + return func(o tidyOptions) tidyOptions { o.output = w + + return o } } @@ -67,8 +77,10 @@ func WithTidyOutput(w io.Writer) TidyOption { // one. Tidying reaches the module proxy and the checksum database, and a generated module often // wants different settings for those than the generator itself. func WithTidyEnv(vars ...string) TidyOption { - return func(o *tidyOptions) { + return func(o tidyOptions) tidyOptions { o.env = append(o.env, vars...) + + return o } } @@ -78,26 +90,30 @@ func WithTidyEnv(vars ...string) TidyOption { // not list it makes the go command refuse to work in it, so [GoGenApp.TidyModule] runs with GOWORK // off. Turn this on for a module the surrounding workspace is meant to cover. func WithWorkspace(enabled bool) TidyOption { - return func(o *tidyOptions) { + return func(o tidyOptions) tidyOptions { o.workspace = enabled + + return o } } // WithTidyWaitDelay bounds how long the command may hold the output pipes open after its context is // done, before it is killed. It defaults to five seconds. func WithTidyWaitDelay(delay time.Duration) TidyOption { - return func(o *tidyOptions) { + return func(o tidyOptions) tidyOptions { o.waitDelay = delay + + return o } } -func tidyOptionsWithDefaults(opts []TidyOption) (tidyOptions, error) { +func applyTidyWithDefaults(opts []TidyOption) (tidyOptions, error) { const defaultWaitDelay = 5 * time.Second o := tidyOptions{goCommand: "go", waitDelay: defaultWaitDelay} for _, apply := range opts { - apply(&o) + o = apply(o) } for _, version := range [...]struct{ flag, value string }{ diff --git a/gentesting/testutils.go b/gentesting/testutils.go index 04d127e..2d0faea 100644 --- a/gentesting/testutils.go +++ b/gentesting/testutils.go @@ -97,24 +97,41 @@ func SanitizeGoModPath(pth string) string { return path.Clean(sanitizer.Replace(filepath.Base(pth))) } -type GoModOption func(o *goModOptions) +type ( + // GoModOption configures [GoModInit]. + GoModOption func(goModOptions) goModOptions -type goModOptions struct { - moduleName string -} + goModOptions struct { + moduleName string + } +) +// WithGoModuleName names the module, instead of deriving it from the path. func WithGoModuleName(name string) GoModOption { - return func(o *goModOptions) { + return func(o goModOptions) goModOptions { o.moduleName = name + + return o } } -func GoModInit(pth string, opts ...GoModOption) func(*testing.T) { +// applyGoModWithDefaults folds the chain over the zero options, left to right. +// +// The zero value leaves the module name empty, and [GoModInit] then derives one from the path with +// [SanitizeGoModPath]. +func applyGoModWithDefaults(opts []GoModOption) goModOptions { var o goModOptions + for _, apply := range opts { - apply(&o) + o = apply(o) } + return o +} + +func GoModInit(pth string, opts ...GoModOption) func(*testing.T) { + o := applyGoModWithDefaults(opts) + if o.moduleName == "" { o.moduleName = SanitizeGoModPath(pth) } From 9c2c8b3d1b54e425e190a94bf82eae4047744c0c Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 23 Aug 2026 22:19:20 +0200 Subject: [PATCH 2/5] fix(formatting): keep every gofumpt rule named in one WithExtraRules call gofumpt's Extra.Set clears itself before reading a rule list, and WithExtraRules called it once per name, so only the last one survived: WithExtraRules("group_params", "clothe_returns") turned group_params back off. Pass the names to Set the way gofumpt's -extra flag takes them, comma separated, in a single call. The tests only ever named one rule, which is why this held. The new one names two and asserts the first survives the second. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- formatting/enable/gofumpt/enable.go | 16 ++++++++++++---- formatting/enable/gofumpt/enable_test.go | 21 +++++++++++++++++++++ 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/formatting/enable/gofumpt/enable.go b/formatting/enable/gofumpt/enable.go index 2042ec7..9d8d054 100644 --- a/formatting/enable/gofumpt/enable.go +++ b/formatting/enable/gofumpt/enable.go @@ -7,6 +7,7 @@ import ( "fmt" "go/ast" "go/token" + "strings" "sync" "github.com/go-openapi/codegen/formatting/internal/rules" @@ -81,12 +82,19 @@ func WithModulePath(path string) Option { // WithExtraRules turns on rules gofumpt leaves off, named as gofumpt names them on its command line: // "group_params", "clothe_returns", "balance_calls". Passing "true" turns all of them on. +// +// The rules named replace whatever an earlier WithExtraRules asked for, which is how gofumpt's own +// -extra flag behaves. Name every rule in one call: +// +// gofumpt.WithExtraRules("group_params", "clothe_returns") func WithExtraRules(rules ...string) Option { return func(o *fumpt.Options) error { - for _, rule := range rules { - if err := o.Extra.Set(rule); err != nil { - return fmt.Errorf("unknown gofumpt rule %q: %w", rule, err) - } + // Extra.Set clears itself before reading a list, so one call per rule would keep only the + // last. It takes the comma-separated form gofumpt's -extra flag takes. + named := strings.Join(rules, ",") + + if err := o.Extra.Set(named); err != nil { + return fmt.Errorf("unknown gofumpt rule in %q: %w", named, err) } return nil diff --git a/formatting/enable/gofumpt/enable_test.go b/formatting/enable/gofumpt/enable_test.go index 8877159..8918036 100644 --- a/formatting/enable/gofumpt/enable_test.go +++ b/formatting/enable/gofumpt/enable_test.go @@ -48,6 +48,27 @@ func TestEnable(t *testing.T) { assert.Contains(t, format(t, source(t, "generated"), formatting.WithGoFumpt()), "func F(a, b int)") }) + t.Run("should apply every rule named in one call", func(t *testing.T) { + // gofumpt's Extra.Set clears itself before reading a list, so asking for the rules one Set at + // a time kept only the last of them. + require.NoError(t, gofumpt.Configure( + gofumpt.WithLangVersion("go1.25"), + gofumpt.WithExtraRules("group_params", "clothe_returns"), + )) + + out := format(t, source(t, "generated"), formatting.WithGoFumpt()) + + assert.Contains(t, out, "func F(a, b int)", + "group_params is named first, and the rule after it must not clear it") + }) + + t.Run("should name the rule it does not know", func(t *testing.T) { + err := gofumpt.Configure(gofumpt.WithExtraRules("group_params", "no_such_rule")) + + require.Error(t, err) + assert.Contains(t, err.Error(), "no_such_rule") + }) + t.Run("should leave the parameters alone without the extra rule", func(t *testing.T) { require.NoError(t, gofumpt.Configure(gofumpt.WithLangVersion("go1.25"))) From 411ce041653ffa5af22c6715c0897077a7cbf69f Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 23 Aug 2026 22:20:13 +0200 Subject: [PATCH 3/5] refact(options): chain the gofumpt options by value WithExtraRules rejects a rule gofumpt does not know, and a chain that passes values has nowhere to put that error. It goes in an options struct of our own, wrapping fumpt.Options beside the first failure: type options struct { fumpt fumpt.Options err error } withError keeps the first failure and lets the rest of the chain run, and Configure reads it before touching the package settings, so a failing option still leaves the previous rules in place. fumpt.Options holds no pointer - Extra is three bools - so the value the chain carries is a plain copy. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- formatting/enable/gofumpt/enable.go | 66 ++++++++++++++++++++--------- 1 file changed, 47 insertions(+), 19 deletions(-) diff --git a/formatting/enable/gofumpt/enable.go b/formatting/enable/gofumpt/enable.go index 9d8d054..de7b039 100644 --- a/formatting/enable/gofumpt/enable.go +++ b/formatting/enable/gofumpt/enable.go @@ -33,24 +33,52 @@ func apply(fset *token.FileSet, file *ast.File) { fumpt.File(fset, file, current) } -// Option configures the gofumpt rules. -type Option func(*fumpt.Options) error +type ( + // Option configures the gofumpt rules. + Option func(options) options + + // options carries the gofumpt settings being assembled, and the first option that rejected its + // arguments. [fumpt.Options] holds no pointer, so the copy the chain passes along is a value. + options struct { + fumpt fumpt.Options + err error + } +) + +// withError keeps the first failure and lets the rest of the chain run. +func (o options) withError(err error) options { + if o.err == nil { + o.err = err + } + + return o +} + +// applyWithDefaults folds the chain over the zero options, left to right. +// +// The zero value is gofumpt's own default: every extra rule off, and a language version of go1. +func applyWithDefaults(opts []Option) options { + var o options + + for _, apply := range opts { + o = apply(o) + } + + return o +} // Configure sets the rules for the whole program. // // Call it once, before formatting anything. It returns an error when an option is not one gofumpt // knows, and leaves the previous settings in place. func Configure(opts ...Option) error { - next := fumpt.Options{} - - for _, apply := range opts { - if err := apply(&next); err != nil { - return err - } + next := applyWithDefaults(opts) + if next.err != nil { + return next.err } mx.Lock() - settings = next + settings = next.fumpt mx.Unlock() return nil @@ -61,10 +89,10 @@ func Configure(opts ...Option) error { // gofumpt holds back the rules that need a language newer than the code targets. Empty means // go1, which holds back all of them. func WithLangVersion(version string) Option { - return func(o *fumpt.Options) error { - o.LangVersion = version + return func(o options) options { + o.fumpt.LangVersion = version - return nil + return o } } @@ -73,10 +101,10 @@ func WithLangVersion(version string) Option { // gofumpt reads it to decide which import paths are outside the standard library when it puts the // standard library imports first. func WithModulePath(path string) Option { - return func(o *fumpt.Options) error { - o.ModulePath = path + return func(o options) options { + o.fumpt.ModulePath = path - return nil + return o } } @@ -88,15 +116,15 @@ func WithModulePath(path string) Option { // // gofumpt.WithExtraRules("group_params", "clothe_returns") func WithExtraRules(rules ...string) Option { - return func(o *fumpt.Options) error { + return func(o options) options { // Extra.Set clears itself before reading a list, so one call per rule would keep only the // last. It takes the comma-separated form gofumpt's -extra flag takes. named := strings.Join(rules, ",") - if err := o.Extra.Set(named); err != nil { - return fmt.Errorf("unknown gofumpt rule in %q: %w", named, err) + if err := o.fumpt.Extra.Set(named); err != nil { + return o.withError(fmt.Errorf("unknown gofumpt rule in %q: %w", named, err)) } - return nil + return o } } From 5b3352c3a785e3cd3762c92eb1e5b872f183d173 Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 23 Aug 2026 22:24:00 +0200 Subject: [PATCH 4/5] refact(options): chain the templates-repo options by value Option, SourceOption and DumpOption took a pointer and returned an error, so the chain stopped at the first failure and the options struct escaped to the heap. They now pass values, and the failure travels in the struct: func (o options) withError(err error) options { if o.err == nil { o.err = err } return o } WithExtensions, WithRoots, WithExtraRoots, WithCoverage, Rebased and the four From* constructors record their first bad argument there. New, Clone and Dump read it before using the settings, so a caller sees the same error from the same call as before. The options after a failing one now run, and none of them touches anything outside the options value. The defaults stay inside makeOptions and applyDumpWithDefaults rather than moving to a package-level value: they hold a FuncMap and a slice, and building them afresh per call is what keeps two repositories from sharing either. For the same reason WithExtraRoots and the From* constructors clone the slice they extend instead of appending in place, which would write into the backing array of whichever chain got there first. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- templates-repo/options.go | 92 +++++++++++++++++++++------------- templates-repo/reports/dump.go | 61 +++++++++++++++------- templates-repo/repository.go | 6 +-- templates-repo/sources.go | 72 +++++++++++++++----------- 4 files changed, 146 insertions(+), 85 deletions(-) diff --git a/templates-repo/options.go b/templates-repo/options.go index 96386a8..0d81a09 100644 --- a/templates-repo/options.go +++ b/templates-repo/options.go @@ -23,16 +23,33 @@ import ( // order: [FromFS], [FromDir] and [FromTemplate]. Settings shape how they are read, whatever the // order: [WithFuncMap], [WithExtensions], [WithRoots], [WithExtraRoots] and [WithCoverage]. // How one source is read is settled where it is declared, with a [SourceOption]. -type Option func(*options) error - -// options holds the settings of a repository, and the sources it is yet to read. -type options struct { - sources []source - funcs template.FuncMap - extensions []string - roots []string - coverPrefix string - coverage bool +type ( + Option func(options) options + + // options holds the settings of a repository, the sources it is yet to read, and the first + // option that rejected its arguments. + options struct { + sources []source + funcs template.FuncMap + extensions []string + roots []string + coverPrefix string + err error + coverage bool + } +) + +// withError keeps the first failure and lets the rest of the chain run. +// +// An option reports a bad argument here rather than from its own constructor, so [New] and [Clone] +// are where a caller sees it - which is where every other reason a repository fails to build shows +// up too. +func (o options) withError(err error) options { + if o.err == nil { + o.err = err + } + + return o } // makeOptions applies opts on top of the defaults. @@ -45,26 +62,25 @@ func makeOptions(opts []Option) (options, error) { extensions: []string{DefaultExtension}, } - if err := o.apply(opts); err != nil { - return options{}, err + o = o.apply(opts) + if o.err != nil { + return options{}, o.err } return o, nil } -// apply runs a list of options over these settings. -func (o *options) apply(opts []Option) error { +// apply folds a list of options over these settings, left to right. +func (o options) apply(opts []Option) options { for _, option := range opts { if option == nil { continue } - if err := option(o); err != nil { - return err - } + o = option(o) } - return nil + return o } // derive copies the settings of a repository for a [Clone], with no source left to read. @@ -91,10 +107,10 @@ func (o options) derive() options { // Adding a function to an existing repository is [Clone] with this option: the clone re-parses // its templates, so the new function reaches all of them. func WithFuncMap(funcs template.FuncMap) Option { - return func(o *options) error { + return func(o options) options { maps.Copy(o.funcs, funcs) - return nil + return o } } @@ -106,14 +122,14 @@ func WithFuncMap(funcs template.FuncMap) Option { // The extension is trimmed from the asset path before its name is derived, so // "validation/primitive.gotmpl" is named validationPrimitive. func WithExtensions(extensions ...string) Option { - return func(o *options) error { + return func(o options) options { if len(extensions) == 0 { - return fmt.Errorf("at least one extension is required: %w", ErrTemplateRepo) + return o.withError(fmt.Errorf("at least one extension is required: %w", ErrTemplateRepo)) } o.extensions = slices.Clone(extensions) - return nil + return o } } @@ -145,15 +161,15 @@ func WithExtensions(extensions ...string) Option { // // the templates a client generation executes, and nothing else // client, err := repo.Clone(repository, repo.WithRoots("clientClient", "clientParameter", "model")) func WithRoots(names ...string) Option { - return func(o *options) error { + return func(o options) options { scope, err := scopeOf(names) if err != nil { - return err + return o.withError(err) } o.roots = scope - return nil + return o } } @@ -171,23 +187,27 @@ func WithRoots(names ...string) Option { // // one more template, reachable whether or not the repository is scoped // mine, err := repo.Clone(repository, repo.FromTemplate("mine", body), repo.WithExtraRoots("mine")) func WithExtraRoots(names ...string) Option { - return func(o *options) error { + return func(o options) options { scope, err := scopeOf(names) if err != nil { - return err + return o.withError(err) } if len(o.roots) == 0 { - return nil // every template is kept already, these among them + return o // every template is kept already, these among them } + widened := slices.Clone(o.roots) + for _, name := range scope { - if !slices.Contains(o.roots, name) { - o.roots = append(o.roots, name) + if !slices.Contains(widened, name) { + widened = append(widened, name) } } - return nil + o.roots = widened + + return o } } @@ -225,14 +245,16 @@ func scopeOf(names []string) ([]string, error) { // // repo.WithCoverage("github.com/go-swagger/go-swagger/generator/templates") func WithCoverage(prefix string) Option { - return func(o *options) error { + return func(o options) options { if strings.TrimSpace(prefix) == "" { - return fmt.Errorf("coverage needs the import path the templates live under: %w", ErrTemplateRepo) + return o.withError( + fmt.Errorf("coverage needs the import path the templates live under: %w", ErrTemplateRepo), + ) } o.coverage = true o.coverPrefix = strings.TrimSuffix(prefix, "/") + "/" - return nil + return o } } diff --git a/templates-repo/reports/dump.go b/templates-repo/reports/dump.go index fde8bde..df8d792 100644 --- a/templates-repo/reports/dump.go +++ b/templates-repo/reports/dump.go @@ -17,11 +17,43 @@ import ( // // Rendering settings belong to the call rather than to the repository: how a document looks is the // business of whoever asks for it, and the template that lays it out is compiled when it is used. -type DumpOption func(*dumpOptions) error +type ( + DumpOption func(dumpOptions) dumpOptions -type dumpOptions struct { - layout string - funcs template.FuncMap + dumpOptions struct { + layout string + funcs template.FuncMap + err error + } +) + +// withError keeps the first failure and lets the rest of the chain run. +func (o dumpOptions) withError(err error) dumpOptions { + if o.err == nil { + o.err = err + } + + return o +} + +// applyDumpWithDefaults folds the chain over the defaults, left to right. +// +// The defaults are built afresh on every call, so no two dumps share the funcmap. +func applyDumpWithDefaults(opts []DumpOption) (dumpOptions, error) { + o := dumpOptions{ + layout: markdownLayout, + funcs: template.FuncMap{"anchor": anchor, "weigh": weigh, "plural": plural}, + } + + for _, apply := range opts { + o = apply(o) + } + + if o.err != nil { + return dumpOptions{}, o.err + } + + return o, nil } // WithTemplate lays the document out with a template of the caller's own. @@ -29,23 +61,23 @@ type dumpOptions struct { // The template is executed against a [Documentation]. It is compiled when [Repository.Dump] runs, // so a template that does not parse is reported by that call. func WithTemplate(text string) DumpOption { - return func(o *dumpOptions) error { + return func(o dumpOptions) dumpOptions { if strings.TrimSpace(text) == "" { - return fmt.Errorf("the dump template is empty: %w", ErrReport) + return o.withError(fmt.Errorf("the dump template is empty: %w", ErrReport)) } o.layout = text - return nil + return o } } // WithFuncMap adds functions a dump template of the caller's own may call. func WithFuncMap(funcs template.FuncMap) DumpOption { - return func(o *dumpOptions) error { + return func(o dumpOptions) dumpOptions { maps.Copy(o.funcs, funcs) - return nil + return o } } @@ -63,14 +95,9 @@ func WithFuncMap(funcs template.FuncMap) DumpOption { // // err = reports.Dump(w, documentation) func Dump(w io.Writer, documentation Documentation, opts ...DumpOption) error { - settings := dumpOptions{ - layout: markdownLayout, - funcs: template.FuncMap{"anchor": anchor, "weigh": weigh, "plural": plural}, - } - for _, apply := range opts { - if err := apply(&settings); err != nil { - return err - } + settings, err := applyDumpWithDefaults(opts) + if err != nil { + return err } layout, err := template.New("dump").Funcs(settings.funcs).Parse(settings.layout) diff --git a/templates-repo/repository.go b/templates-repo/repository.go index d8eff50..abf91e6 100644 --- a/templates-repo/repository.go +++ b/templates-repo/repository.go @@ -112,9 +112,9 @@ func Clone(source *Repository, opts ...Option) (*Repository, error) { return nil, fmt.Errorf("cannot clone a nil repository: %w", ErrTemplateRepo) } - settings := source.settings.derive() - if err := settings.apply(opts); err != nil { - return nil, err + settings := source.settings.derive().apply(opts) + if settings.err != nil { + return nil, settings.err } added, layers, err := settings.resolveSources(source.layers) diff --git a/templates-repo/sources.go b/templates-repo/sources.go index a6c7af1..3090a3a 100644 --- a/templates-repo/sources.go +++ b/templates-repo/sources.go @@ -47,18 +47,18 @@ type source func(options) ([]asset, error) // fileutils.MustSub(assets, "templates/contrib/mine"), // ), "")) func FromFS(fsys fs.FS, mountPoint string, opts ...SourceOption) Option { - return func(o *options) error { + return func(o options) options { mount, err := cleanMountPoint(mountPoint) if err != nil { - return err + return o.withError(err) } reading, err := makeSourceOptions(opts) if err != nil { - return err + return o.withError(err) } - o.sources = append(o.sources, func(settings options) ([]asset, error) { + o.sources = append(slices.Clip(o.sources), func(settings options) ([]asset, error) { if fsys == nil { return nil, fmt.Errorf("cannot read templates from a nil fs.FS: %w", ErrTemplateRepo) } @@ -66,7 +66,7 @@ func FromFS(fsys fs.FS, mountPoint string, opts ...SourceOption) Option { return readFS(fsys, reading.mount(mount), reading.skipDirectories, settings) }) - return nil + return o } } @@ -87,7 +87,7 @@ func FromFS(fsys fs.FS, mountPoint string, opts ...SourceOption) Option { // ) // } func Sources(opts ...Option) Option { - return func(o *options) error { + return func(o options) options { return o.apply(opts) } } @@ -97,12 +97,22 @@ func Sources(opts ...Option) Option { // Which directories to skip describes the file system being walked, not the repository. Skipping a // directory of the assets one source ships leaves a directory of the same name fully readable in // a template set someone else brings. -type SourceOption func(*sourceOptions) error +type SourceOption func(sourceOptions) sourceOptions // sourceOptions holds the settings of a single source. type sourceOptions struct { skipDirectories []string rebase string + err error +} + +// withError keeps the first failure and lets the rest of the chain run. +func (o sourceOptions) withError(err error) sourceOptions { + if o.err == nil { + o.err = err + } + + return o } // Rebased mounts a source under a base, on top of wherever it already mounts. @@ -123,15 +133,15 @@ type sourceOptions struct { // genclient.Sources(repo.Rebased("client")), // ) func Rebased(base string) SourceOption { - return func(o *sourceOptions) error { + return func(o sourceOptions) sourceOptions { under, err := cleanMountPoint(base) if err != nil { - return err + return o.withError(err) } o.rebase = path.Join(o.rebase, under) - return nil + return o } } @@ -149,9 +159,11 @@ func makeSourceOptions(opts []SourceOption) (sourceOptions, error) { continue } - if err := option(&o); err != nil { - return sourceOptions{}, err - } + o = option(o) + } + + if o.err != nil { + return sourceOptions{}, o.err } return o, nil @@ -168,10 +180,10 @@ func makeSourceOptions(opts []SourceOption) (sourceOptions, error) { // // the assets shipped, leaving the alternate sets to be stacked explicitly // repo.FromFS(assets, "", repo.SkipDirectories("contrib")) func SkipDirectories(names ...string) SourceOption { - return func(o *sourceOptions) error { + return func(o sourceOptions) sourceOptions { o.skipDirectories = append(o.skipDirectories, names...) - return nil + return o } } @@ -184,18 +196,18 @@ func SkipDirectories(names ...string) SourceOption { // The directory is read once, when the repository is built. Editing a template on disk // afterwards has no effect until a repository is built again. func FromDir(dir, mountPoint string, opts ...SourceOption) Option { - return func(o *options) error { + return func(o options) options { mount, err := cleanMountPoint(mountPoint) if err != nil { - return err + return o.withError(err) } reading, err := makeSourceOptions(opts) if err != nil { - return err + return o.withError(err) } - o.sources = append(o.sources, func(settings options) ([]asset, error) { + o.sources = append(slices.Clip(o.sources), func(settings options) ([]asset, error) { info, err := os.Stat(dir) if err != nil { return nil, fmt.Errorf("cannot read templates from %q: %w: %w", dir, err, ErrTemplateRepo) @@ -208,7 +220,7 @@ func FromDir(dir, mountPoint string, opts ...SourceOption) Option { return readFS(os.DirFS(dir), reading.mount(mount), reading.skipDirectories, settings) }) - return nil + return o } } @@ -235,20 +247,20 @@ func FromDir(dir, mountPoint string, opts ...SourceOption) Option { // repo.FromRepository(serverTemplates, "server"), // ) func FromRepository(source *Repository, mountPoint string, opts ...SourceOption) Option { - return func(o *options) error { + return func(o options) options { mount, err := cleanMountPoint(mountPoint) if err != nil { - return err + return o.withError(err) } reading, err := makeSourceOptions(opts) if err != nil { - return err + return o.withError(err) } mount = reading.mount(mount) - o.sources = append(o.sources, func(options) ([]asset, error) { + o.sources = append(slices.Clip(o.sources), func(options) ([]asset, error) { if source == nil { return nil, fmt.Errorf("cannot read templates from a nil repository: %w", ErrTemplateRepo) } @@ -262,7 +274,7 @@ func FromRepository(source *Repository, mountPoint string, opts ...SourceOption) return read, nil }) - return nil + return o } } @@ -280,24 +292,24 @@ func FromRepository(source *Repository, mountPoint string, opts ...SourceOption) // // The content is retained, not copied. func FromTemplate(name string, content []byte, opts ...SourceOption) Option { - return func(o *options) error { + return func(o options) options { clean, err := cleanAssetName(name) if err != nil { - return err + return o.withError(err) } reading, err := makeSourceOptions(opts) if err != nil { - return err + return o.withError(err) } clean = reading.mount(clean) - o.sources = append(o.sources, func(options) ([]asset, error) { + o.sources = append(slices.Clip(o.sources), func(options) ([]asset, error) { return []asset{{path: clean, data: content}}, nil }) - return nil + return o } } From 45259d220ac805c6b7d363a75abdaa9790513cec Mon Sep 17 00:00:00 2001 From: Frederic BIDON Date: Sun, 23 Aug 2026 22:29:12 +0200 Subject: [PATCH 5/5] doc(templates-repo): say that a bad DumpOption is reported by Dump Option in the repository package states where a rejected argument surfaces, and DumpOption did not. It behaves the same way: the option records the failure and Dump returns it. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Frederic BIDON --- templates-repo/reports/dump.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/templates-repo/reports/dump.go b/templates-repo/reports/dump.go index df8d792..370fd61 100644 --- a/templates-repo/reports/dump.go +++ b/templates-repo/reports/dump.go @@ -17,6 +17,9 @@ import ( // // Rendering settings belong to the call rather than to the repository: how a document looks is the // business of whoever asks for it, and the template that lays it out is compiled when it is used. +// +// An option that cannot be honoured reports an error from [Dump], rather than at the point where it +// is constructed. type ( DumpOption func(dumpOptions) dumpOptions