Skip to content

Lower-impact polish (bundled checklist) #54

@Ilyes512

Description

@Ilyes512

Parent: #42 (architectural review)

Context

This issue bundles small, independent polish items from the architectural review. Each item is small enough that a separate issue would be overhead — pick any one and ship it. They are unrelated, so a single PR that fixes one is fine; do not bundle these into one mega-PR.

Checklist

pkg/template/functions.go:74panic on registry registration failure

The current code panics on handler.AddRegistries(...) failure. Acceptable, but a must-style helper is more idiomatic in Go and easier to grep. Replace with mustAddRegistries or similar.

  • Extract to a helper named must….
  • One line wrapping the call; preserves the panic semantics for programmer error.

pkg/cmd/template_list.go:153,157 — silent JSON unmarshal swallow

loadMetadataForListing returns nil, nil on both missing-file and malformed-JSON. Missing is fine (annotated //nolint:nilerr), but malformed metadata is a real diagnostic signal that's being dropped.


pkg/cmd/use.go:65 — load-bearing constraint hidden in a comment

The comment "go-git requires the destination to not exist; use a subdirectory" describes a constraint that will silently break if a future refactor removes the subdirectory step.

  • Encode the constraint in the type system: introduce pkggit.CloneInto(parent, name string, opts CloneOptions) (cloneDir string, err error) that internally guarantees the destination doesn't pre-exist.
  • Update runUse and any other callers to use the new helper.
  • Remove the explanatory comment (the helper's signature now documents it).

pkg/template/template.go:43 — hard-coded ignoredFiles map

{".DS_Store": true, "Thumbs.db": true} is reasonable but inflexible. Consider:

  • Make the map exported (or document it) so the docs at least list what's silently skipped.
  • Optionally support a .specsignore file (gitignore-style) for per-template additions. Be careful: the existing .specsverbatim already exists; don't duplicate semantics. .specsignore would mean "do not copy at all"; .specsverbatim means "copy but don't render".
  • At minimum: document the current behavior in docs/content/docs/architecture/template-engine.md.

go.modgo 1.26.1 is bleeding edge

Not a problem today, but it restricts go install github.com/specsnl/specs-cli@latest for users on older Go versions. Two options:

  • Lower to go 1.25 (or oldest still-supported) if no language features beyond that are used.
  • If 1.26 features are used (verify with go vet), document the requirement prominently in README under Installation.

pkg/cmd/app.goApp mutable fields are race-prone

App.SafeMode and App.HookEnvPrefix are mutated by PersistentPreRunE (pkg/cmd/root.go:32-37). Fine for single-shot CLI invocations, but it's an awkward pattern that complicates testing (tests must reset state between cases). A typed config struct passed explicitly to commands would be cleaner.

  • Introduce cmd.RunConfig (or similar) holding SafeMode, HookEnvPrefix, etc.
  • PersistentPreRunE builds a *RunConfig and stashes it on the cobra command's context (cmd.SetContext(ctx)).
  • Commands read from ctx, not from App. App keeps only truly app-wide singletons (Logger, Output).

Acceptance criteria

Each unchecked box above is a candidate task. None are blockers for the main review (#42). Consider closing this issue once half are done, and opening fresh issues for the rest if they grow in scope.

References

See the line/file references inside each section.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions