Decide on renaming internal/log, internal/crypto and internal/types (revive var-naming) #76

New Issue

clawbot · 2026-08-09T03:45:00+02:00

clawbot commented

2026-08-09 03:45:00 +02:00

Split out of #61 so the lint-remediation PR does not make a repo-wide
naming decision on its own.

revive's var-naming rule reports three package-name findings that
cannot be fixed without renaming packages across the whole codebase:

internal/log — "avoid package names that conflict with Go standard
library package names" (log)
internal/crypto — same, conflicts with stdlib crypto
internal/types — "avoid meaningless package names"

In the lint-remediation work these are suppressed with per-site
//nolint:revive directives on the package clauses, because the
alternative is a rename that touches nearly every file in the repo and
changes import paths — a design decision for the repo owner, not a
mechanical lint fix.

Context that argues the suppressions are safe today:

Neither stdlib log nor stdlib crypto is imported anywhere in the
repo. internal/log only imports log/slog, which binds the
identifier slog, so there is no actual shadowing conflict in any
file.
internal/types holds the ID/hash/path/credential wrapper types; a
more specific name is possible but is a naming call.

Implementation note for whoever picks this up: the directives carry
nolintlint alongside revive. revive reports the package-name
failure only once per package directory, on whichever file of the
package the linter happens to process first (revive lints a package's
files concurrently over a map), so the finding can land on any file in
the package. Every file of the affected packages therefore carries the
directive, and nolintlint is listed so the directives on the files
that did not win the race are not reported as unused.

Definition of done

A decision is recorded: either rename (with the chosen names) or
keep the current names permanently.
If renaming: packages renamed, all importers updated, make check
still exits 0, and the //nolint:revive,nolintlint directives on
the package clauses of internal/log, internal/crypto and
internal/types are removed.
If keeping: this issue is closed with the rationale, and the
existing directives stay as they are.

Split out of #61 so the lint-remediation PR does not make a repo-wide naming decision on its own. `revive`'s `var-naming` rule reports three package-name findings that cannot be fixed without renaming packages across the whole codebase: - `internal/log` — "avoid package names that conflict with Go standard library package names" (`log`) - `internal/crypto` — same, conflicts with stdlib `crypto` - `internal/types` — "avoid meaningless package names" In the lint-remediation work these are suppressed with per-site `//nolint:revive` directives on the package clauses, because the alternative is a rename that touches nearly every file in the repo and changes import paths — a design decision for the repo owner, not a mechanical lint fix. Context that argues the suppressions are safe today: - Neither stdlib `log` nor stdlib `crypto` is imported anywhere in the repo. `internal/log` only imports `log/slog`, which binds the identifier `slog`, so there is no actual shadowing conflict in any file. - `internal/types` holds the ID/hash/path/credential wrapper types; a more specific name is possible but is a naming call. Implementation note for whoever picks this up: the directives carry `nolintlint` alongside `revive`. `revive` reports the package-name failure only once per package directory, on whichever file of the package the linter happens to process first (revive lints a package's files concurrently over a map), so the finding can land on any file in the package. Every file of the affected packages therefore carries the directive, and `nolintlint` is listed so the directives on the files that did not win the race are not reported as unused. ## Definition of done 1. A decision is recorded: either rename (with the chosen names) or keep the current names permanently. 2. If renaming: packages renamed, all importers updated, `make check` still exits 0, and the `//nolint:revive,nolintlint` directives on the package clauses of `internal/log`, `internal/crypto` and `internal/types` are removed. 3. If keeping: this issue is closed with the rationale, and the existing directives stay as they are.

clawbot referenced this issue from a commit

2026-08-09 03:49:26 +02:00

Suppress the gosec and revive findings with no fix (closes #61)

clawbot referenced this issue

2026-08-09 03:49:49 +02:00

Finish the lint remediation: script/cibuild exits 0 (closes #61) #77

clawbot referenced this issue

2026-08-09 03:50:06 +02:00

Remediate lint findings surfaced by the standard .golangci.yml #61

clawbot commented

2026-08-09 03:52:01 +02:00

Manager note — routing this to sneak, since it is purely a naming call.

My recommendation: keep the current names and close this issue, i.e.
option 3 in the definition of done.

Reasoning:

Nothing is actually shadowed. Neither stdlib log nor stdlib crypto
is imported anywhere in the repo, and internal/log imports
log/slog, which binds slog. The rule is guarding against a
collision that does not exist here.
These are internal/ packages, so no external consumer can import
them. There is no compatibility cost to renaming later, which means
this is not a 1.0 blocker — I have deliberately left it off the
1.0.0 milestone.
internal/log and internal/crypto are the clearest possible names
for what they hold. Renaming to something like vlog/vcrypto to
satisfy a linter would make the code worse to read in exchange for
nothing.

The one I would not defend as strongly is internal/types —
"meaningless package name" is a fair hit, and internal/ids or
internal/idtypes would describe the ID/hash/path/credential wrappers
better. But that rename touches a lot of files for a modest readability
gain, and #73 (security review) is scheduled to add tests to that package
soon; churning its name first would create conflicts for no benefit.

So: keep all three, close this, leave the four //nolint:revive,nolintlint
directives in place. If you would rather rename internal/types
specifically, say so and I will schedule it after #73 lands rather than
before.

Manager note — routing this to `sneak`, since it is purely a naming call. My recommendation: **keep the current names and close this issue**, i.e. option 3 in the definition of done. Reasoning: 1. Nothing is actually shadowed. Neither stdlib `log` nor stdlib `crypto` is imported anywhere in the repo, and `internal/log` imports `log/slog`, which binds `slog`. The rule is guarding against a collision that does not exist here. 2. These are `internal/` packages, so no external consumer can import them. There is no compatibility cost to renaming later, which means this is **not a 1.0 blocker** — I have deliberately left it off the `1.0.0` milestone. 3. `internal/log` and `internal/crypto` are the clearest possible names for what they hold. Renaming to something like `vlog`/`vcrypto` to satisfy a linter would make the code worse to read in exchange for nothing. The one I would not defend as strongly is **`internal/types`** — "meaningless package name" is a fair hit, and `internal/ids` or `internal/idtypes` would describe the ID/hash/path/credential wrappers better. But that rename touches a lot of files for a modest readability gain, and #73 (security review) is scheduled to add tests to that package soon; churning its name first would create conflicts for no benefit. So: keep all three, close this, leave the four `//nolint:revive,nolintlint` directives in place. If you would rather rename `internal/types` specifically, say so and I will schedule it after #73 lands rather than before.

sneak was assigned by clawbot

2026-08-09 03:52:03 +02:00

clawbot referenced this issue

2026-08-09 03:58:02 +02:00

Finish the lint remediation: script/cibuild exits 0 (closes #61) #77

clawbot referenced this issue

2026-08-09 03:59:24 +02:00

Finish the lint remediation: script/cibuild exits 0 (closes #61) #77

clawbot referenced this issue from a commit

2026-08-09 04:14:11 +02:00

Suppress the revive package-name findings with no fix (closes #61)

clawbot referenced this issue