Change NewChecker to take a Params struct instead of positional arguments #78

New Issue

clawbot · 2026-08-09T03:43:07+02:00

clawbot commented

2026-08-09 03:43:07 +02:00

Context

The Go style guide: "Constructors must take a Params struct... even
for a single argument... Positional arguments for constructors are an
endless source of bugs."

mfer/checker.go:88:

func NewChecker(manifestPath string, basePath string, fs afero.Fs) (*Checker, error)

Three positional arguments, the first two of which are same-typed strings
that are easy to transpose and impossible for the compiler to catch. This is
precisely the failure mode the rule exists to prevent, and it is on a
constructor that is part of the library's public API — so it must be fixed
before 1.0 freezes it.

mfer/scanner.go:86 already does this correctly:
func NewScannerWithOptions(opts *ScannerOptions) *Scanner. The codebase is
internally inconsistent.

Definition of done

NewChecker takes a params struct with named fields for the manifest
path, base path, and filesystem.
The struct's naming is consistent with the existing ScannerOptions
convention in the same package — do not introduce a second naming style
for the same concept.
All call sites are updated, including tests.
Zero-value and missing-field behavior is defined and tested: what happens
when the filesystem field is nil (default to the OS filesystem, or error),
and what happens when a required path is empty (error, with a clear
message).
make check passes. TODO.md updated in the same commit.

Implementation requirements

This is a public API break in a pre-1.0 library, which is exactly when it
should happen. Do not add a compatibility shim or keep the old positional
constructor alongside the new one — that would preserve the bug class into
1.0.
While you are here, audit every other exported constructor in the mfer
package for the same violation and fix them in the same PR. A partial
migration is worse than none because it leaves the convention ambiguous.
List what you found and what you changed in the commit message.
Do not change Checker's behavior. This is a signature change only; any
behavior difference is a bug in the change.
Named-field construction at call sites is required — do not update call
sites to use positional struct literals, which reintroduces exactly the
ordering hazard being removed.
Commit title must end with (closes #78).

## Context The Go style guide: "Constructors **must** take a `Params` struct... even for a single argument... Positional arguments for constructors are an endless source of bugs." `mfer/checker.go:88`: ```go func NewChecker(manifestPath string, basePath string, fs afero.Fs) (*Checker, error) ``` Three positional arguments, the first two of which are same-typed `string`s that are easy to transpose and impossible for the compiler to catch. This is precisely the failure mode the rule exists to prevent, and it is on a constructor that is part of the library's public API — so it must be fixed before 1.0 freezes it. `mfer/scanner.go:86` already does this correctly: `func NewScannerWithOptions(opts *ScannerOptions) *Scanner`. The codebase is internally inconsistent. ## Definition of done - `NewChecker` takes a params struct with named fields for the manifest path, base path, and filesystem. - The struct's naming is consistent with the existing `ScannerOptions` convention in the same package — do not introduce a second naming style for the same concept. - All call sites are updated, including tests. - Zero-value and missing-field behavior is defined and tested: what happens when the filesystem field is nil (default to the OS filesystem, or error), and what happens when a required path is empty (error, with a clear message). - `make check` passes. `TODO.md` updated in the same commit. ## Implementation requirements - This is a public API break in a pre-1.0 library, which is exactly when it should happen. Do not add a compatibility shim or keep the old positional constructor alongside the new one — that would preserve the bug class into 1.0. - While you are here, audit every other exported constructor in the `mfer` package for the same violation and fix them in the same PR. A partial migration is worse than none because it leaves the convention ambiguous. List what you found and what you changed in the commit message. - Do not change `Checker`'s behavior. This is a signature change only; any behavior difference is a bug in the change. - Named-field construction at call sites is required — do not update call sites to use positional struct literals, which reintroduces exactly the ordering hazard being removed. - Commit title must end with ` (closes #78)`.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:43:07 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/mfer#78