From b80b4301ec2197fb625c6d5be63e1832e5f91008 Mon Sep 17 00:00:00 2001 From: Ilyes512 Date: Fri, 4 Sep 2026 09:10:44 +0200 Subject: [PATCH] docs(comments): drop comments that carry nothing the code does not MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The repo's convention is a comment that explains why, and most of them earn their place. These do not. Three kinds are removed. Diff narration — templateRow explaining that `Name` "was a JSON key only because it was also a heading", and executeTemplate naming the Phase 7 and Phase 8 that introduced it — records what changed, not what is. That belongs in a commit message; in the code the next reader has no idea what "was" refers to. Restatement — "Check file content." above os.ReadFile, "Build dependency graph among computed keys." above the loop that builds it, the numbered walkthrough of Execute's walk function, "validateGitHubName checks that a GitHub owner or repo name is valid." The two "copy so the caller's map is not mutated" notes in context.go say what the function doc two paragraphs up already promises. Duplication — the rationale for splitting display strings from row values was written out four times, once per site it touched. It is kept where the split is defined (Column and TableData) and the other sites point at it rather than restating it, so rewording it later is one edit and not four. Where a comment mixed a real constraint into a restatement the constraint is kept and the restatement dropped: hasEmptySegment's "conditional directory exclusion" becomes the subtree exclusion it actually means, and validate.go keeps the non-obvious half about Template.Referenced covering computed expressions. No behaviour changes, so no test or doc updates accompany this. --- internal/cmd/app.go | 10 +++------- internal/cmd/template_list.go | 5 ----- internal/cmd/template_use.go | 8 +++----- internal/cmd/template_use_test.go | 11 +++-------- internal/host/source.go | 1 - internal/template/context.go | 4 ---- internal/template/template.go | 7 ++----- internal/template/validate.go | 9 +++------ internal/util/output/rows.go | 3 +-- internal/util/output/tty.go | 2 +- internal/util/output/writer.go | 4 +--- 11 files changed, 17 insertions(+), 47 deletions(-) diff --git a/internal/cmd/app.go b/internal/cmd/app.go index 76adb44..c625a7f 100644 --- a/internal/cmd/app.go +++ b/internal/cmd/app.go @@ -40,13 +40,9 @@ type App struct { refreshTimeout time.Duration } -// NewApp creates an App. -// -// The logger it installs is silent: nothing is emitted until PersistentPreRunE -// calls output.SetupLogger again with the resolved --debug and --output flags. -// This first call exists only so that a failure before flag parsing has a -// default logger to reach, and it writes to os.Stderr because no command — and -// therefore no cmd.ErrOrStderr() — exists yet. +// NewApp creates an App. The silent logger it installs writes to os.Stderr +// because no command — and therefore no cmd.ErrOrStderr() — exists yet; see +// output.SetupLogger for why it is installed twice. func NewApp() *App { output.SetupLogger(os.Stderr, output.FormatPretty, false) diff --git a/internal/cmd/template_list.go b/internal/cmd/template_list.go index af901bc..a4289bf 100644 --- a/internal/cmd/template_list.go +++ b/internal/cmd/template_list.go @@ -32,11 +32,6 @@ func isTrackable(meta *pkgtemplate.Metadata) bool { // templateRow is one row of `template list`. // -// The json tags are what a consumer's jq filter matches on; the column headings -// beside them are prose for a reader and can be reworded without changing a -// single key. That separation is the point of the type: one string used to do -// both jobs, and `Name` was a JSON key only because it was also a heading. -// // Created and Updated carry the timestamps themselves rather than the "3 days // ago" the table shows — a relative phrase is a reading aid, not something a // script can compute with. A field with no value is absent rather than "-", diff --git a/internal/cmd/template_use.go b/internal/cmd/template_use.go index 543e988..1d39db3 100644 --- a/internal/cmd/template_use.go +++ b/internal/cmd/template_use.go @@ -65,8 +65,8 @@ func newTemplateUseCmd(app *App) *cobra.Command { return cmd } -// executeTemplate is the shared execution logic reused by specs template use (Phase 7) -// and specs use (Phase 8). +// executeTemplate is the shared execution logic reused by `specs template use` +// and `specs use`. func (a *App) executeTemplate(templateRoot, targetDir string, opts executeOpts) error { cfg := a.templateConfig() cfg.ContinueOnRenderError = opts.continueOnError @@ -143,7 +143,6 @@ func (a *App) executeTemplate(templateRoot, targetDir string, opts executeOpts) } } - // Emit one log line per key in alphabetical order with its final resolved source. for _, k := range sortedKeys(finalSource) { slog.Debug("context key resolved", "key", k, "source", finalSource[k]) } @@ -364,7 +363,6 @@ func promptContext( // Iterative conditional passes: each round handles one dependency layer. for len(remaining) > 0 { - // Find keys whose gate variables are all resolved (or not in schema). var ready []string for k := range remaining { @@ -415,7 +413,7 @@ func promptContext( // // When there is nothing that could answer the form, it returns an error naming // every key it would have asked for rather than blocking on a read nobody will -// answer — the CI hang this guard exists to prevent. +// answer. See prompter.cannotPrompt. func runPromptPass( p prompter, ctx map[string]any, diff --git a/internal/cmd/template_use_test.go b/internal/cmd/template_use_test.go index 5304a85..00cc6e1 100644 --- a/internal/cmd/template_use_test.go +++ b/internal/cmd/template_use_test.go @@ -680,8 +680,6 @@ func TestTemplateUse_SafeMode_SkipsHooks(t *testing.T) { } } -// TestExecuteTemplate_RemoteHooks_RunsWithYes verifies that when remote=true and yes=true, -// hooks execute without an interactive prompt. func TestExecuteTemplate_RemoteHooks_RunsWithYes(t *testing.T) { dir := t.TempDir() sentinel := filepath.Join(t.TempDir(), "hook-ran") @@ -705,8 +703,7 @@ func TestExecuteTemplate_RemoteHooks_RunsWithYes(t *testing.T) { } } -// TestExecuteTemplate_RemoteHooks_SafeMode verifies that safe-mode skips hooks -// even for remote sources (no confirmation needed). +// Safe mode skips hooks before the remote confirmation is ever reached. func TestExecuteTemplate_RemoteHooks_SafeMode(t *testing.T) { dir := t.TempDir() sentinel := filepath.Join(t.TempDir(), "hook-ran") @@ -729,7 +726,6 @@ func TestExecuteTemplate_RemoteHooks_SafeMode(t *testing.T) { } } -// TestExecuteTemplate_SafeMode_AllowHooks verifies that --allow-hooks overrides safe-mode. func TestExecuteTemplate_SafeMode_AllowHooks(t *testing.T) { dir := t.TempDir() sentinel := filepath.Join(t.TempDir(), "hook-ran") @@ -779,9 +775,8 @@ func TestTemplateUse_AmbiguousProjectFiles(t *testing.T) { } } -// A non-interactive run that still needs a value fails immediately, naming the -// variable, rather than blocking on a form nobody can answer. That hang — with -// no output explaining it — is what this guard exists to prevent. +// A run that still needs a value fails immediately, naming the variable, rather +// than blocking on a form nobody can answer. func TestTemplateUse_NoTerminal_NamesTheMissingVariables(t *testing.T) { withTempRegistry(t) src := makeTemplateWithVar(t, "Name", "world") diff --git a/internal/host/source.go b/internal/host/source.go index 85aa661..e6d645e 100644 --- a/internal/host/source.go +++ b/internal/host/source.go @@ -135,7 +135,6 @@ func parseGitHub(rest, input string) (*Source, error) { return s, nil } -// validateGitHubName checks that a GitHub owner or repo name is valid. func validateGitHubName(kind, name string, maxLen int) error { if name == "" { return fmt.Errorf("%s is empty", kind) diff --git a/internal/template/context.go b/internal/template/context.go index ad6dd22..374859e 100644 --- a/internal/template/context.go +++ b/internal/template/context.go @@ -134,7 +134,6 @@ func ApplyComputed(ctx map[string]any, defs map[string]string, funcMap texttempl return ctx, nil } - // Build dependency graph among computed keys. keys := make([]string, 0, len(defs)) for k := range defs { keys = append(keys, k) @@ -150,7 +149,6 @@ func ApplyComputed(ctx map[string]any, defs map[string]string, funcMap texttempl return nil, fmt.Errorf("computed values: %w", err) } - // Copy the context so we don't mutate the caller's map. result := make(map[string]any, len(ctx)+len(defs)) maps.Copy(result, ctx) @@ -299,7 +297,6 @@ func extractComputed(raw map[string]any) (map[string]string, error) { // topological order so that each key's pre-fill value is correct before the user is prompted. // It returns a new map; the caller's input is never modified. func resolveReferencedDefaults(ctx map[string]any, funcMap texttemplate.FuncMap, delims specs.Delimiters) (map[string]any, error) { - // Find keys whose string value is a template expression. var refKeys []string for k, v := range ctx { @@ -322,7 +319,6 @@ func resolveReferencedDefaults(ctx map[string]any, funcMap texttemplate.FuncMap, return nil, fmt.Errorf("referenced defaults: %w", err) } - // Copy the input map so the caller's map is not mutated. result := make(map[string]any, len(ctx)) maps.Copy(result, ctx) diff --git a/internal/template/template.go b/internal/template/template.go index c86234e..714638c 100644 --- a/internal/template/template.go +++ b/internal/template/template.go @@ -236,7 +236,6 @@ func (t *Template) Execute(targetDir string) error { return nil // skip the root itself } - // 1. Skip OS/editor metadata files. if ignoredFiles[d.Name()] { if d.IsDir() { return filepath.SkipDir @@ -245,7 +244,6 @@ func (t *Template) Execute(targetDir string) error { return nil } - // 2. Render the relative path as a template to get the destination path. destRel, err := t.renderName(rel, ctx) if err != nil || strings.TrimSpace(destRel) == "" { slog.Debug("skipping path", "path", rel, "error", err) @@ -261,7 +259,8 @@ func (t *Template) Execute(targetDir string) error { return nil } - // 3. Skip if any path segment is empty (conditional directory exclusion). + // An empty path segment means a conditional rendered away a directory + // name, which excludes the whole subtree. if hasEmptySegment(destRel) { if !d.IsDir() { skipped++ @@ -276,12 +275,10 @@ func (t *Template) Execute(targetDir string) error { destPath := filepath.Join(targetDir, destRel) - // 4. Directory: create it. if d.IsDir() { return os.MkdirAll(destPath, 0755) } - // 5. File: determine copy strategy. relForward := filepath.ToSlash(rel) if t.verbatim.Matches(relForward) || isBinary(srcPath) { slog.Debug("file decision", "path", rel, "dest", destPath, "action", "verbatim") diff --git a/internal/template/validate.go b/internal/template/validate.go index 74257c4..beb4a43 100644 --- a/internal/template/validate.go +++ b/internal/template/validate.go @@ -98,7 +98,6 @@ func (t *Template) Validate() ([]ValidationIssue, error) { } } - // Scan template files and path expressions for unknown variable references. srcRoot := filepath.Join(t.Root, specs.TemplateDirFile) err := filepath.WalkDir(srcRoot, func(path string, d fs.DirEntry, err error) error { @@ -113,7 +112,7 @@ func (t *Template) Validate() ([]ValidationIssue, error) { reportPath := filepath.ToSlash(relFromRoot) - // Check the entry's name as a path template expression. + // A file or directory name is itself a path template expression. for _, k := range extractRefs(d.Name(), t.funcMap, t.cfg.delims()) { if !allDefined[k] { addIssue(ErrUnknownVariable, k, reportPath) @@ -124,7 +123,6 @@ func (t *Template) Validate() ([]ValidationIssue, error) { return nil } - // Check file content. data, readErr := os.ReadFile(path) if readErr != nil { return readErr @@ -142,15 +140,14 @@ func (t *Template) Validate() ([]ValidationIssue, error) { return nil, err } - // Unused variables: defined in project.yaml but not referenced anywhere. - // Template.Referenced already includes variables used in computed expressions. + // Template.Referenced already includes variables used in computed + // expressions. for k := range t.Context { if !t.Referenced[k] { addIssue(ErrUnusedVariable, k, "") } } - // Unused computed: defined under "computed" but not referenced anywhere. for k := range t.ComputedDefs { if !t.Referenced[k] { addIssue(ErrUnusedComputed, k, "") diff --git a/internal/util/output/rows.go b/internal/util/output/rows.go index 4ed72d7..1c9e36a 100644 --- a/internal/util/output/rows.go +++ b/internal/util/output/rows.go @@ -47,8 +47,7 @@ type TableData struct { // // It is a function rather than a Writer method because Go has no generic // methods. Going through it is also what guarantees every row has exactly one -// cell per header — alignment a raw [][]Cell can silently get wrong — and what -// keeps the two forms of a row derived from the same value. +// cell per header — alignment a raw [][]Cell can silently get wrong. func Table[T any](w Writer, rows []T, cols ...Column[T]) { data := TableData{ Headers: make([]string, len(cols)), diff --git a/internal/util/output/tty.go b/internal/util/output/tty.go index a878c64..9695be9 100644 --- a/internal/util/output/tty.go +++ b/internal/util/output/tty.go @@ -10,7 +10,7 @@ type fileDescriptor interface { // IsTTY reports whether stream is an interactive terminal. // // The parameter is any rather than io.Writer deliberately: the question that -// matters most here is about **stdin**, because the failure being prevented is a +// matters most here is about stdin, because the failure being prevented is a // read with nobody to answer it. A job with a terminal on stderr and its stdin // closed must still refuse to prompt, and cmd.InOrStdin() hands back an // io.Reader. diff --git a/internal/util/output/writer.go b/internal/util/output/writer.go index 0909df9..9873361 100644 --- a/internal/util/output/writer.go +++ b/internal/util/output/writer.go @@ -240,9 +240,7 @@ func (w *JSONWriter) WriteErr(err error) { fmt.Fprintln(w.stderr, string(data)) } -// WriteTable emits one JSON object per row: the row values themselves, so a -// number stays a number and the keys come from the row type's json tags rather -// than from a column heading written for a reader. +// WriteTable emits one JSON object per row. // // One object per line, not one array. An array cannot be parsed until its // closing bracket arrives, so a run that is killed or fails partway leaves