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.
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 clawbot2026-08-09 03:52:03 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Split out of #61 so the lint-remediation PR does not make a repo-wide
naming decision on its own.
revive'svar-namingrule reports three package-name findings thatcannot be fixed without renaming packages across the whole codebase:
internal/log— "avoid package names that conflict with Go standardlibrary package names" (
log)internal/crypto— same, conflicts with stdlibcryptointernal/types— "avoid meaningless package names"In the lint-remediation work these are suppressed with per-site
//nolint:revivedirectives on the package clauses, because thealternative 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:
lognor stdlibcryptois imported anywhere in therepo.
internal/logonly importslog/slog, which binds theidentifier
slog, so there is no actual shadowing conflict in anyfile.
internal/typesholds the ID/hash/path/credential wrapper types; amore specific name is possible but is a naming call.
Implementation note for whoever picks this up: the directives carry
nolintlintalongsiderevive.revivereports the package-namefailure 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
nolintlintis listed so the directives on the filesthat did not win the race are not reported as unused.
Definition of done
keep the current names permanently.
make checkstill exits 0, and the
//nolint:revive,nolintlintdirectives onthe package clauses of
internal/log,internal/cryptoandinternal/typesare removed.existing directives stay as they are.
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:
lognor stdlibcryptois imported anywhere in the repo, and
internal/logimportslog/slog, which bindsslog. The rule is guarding against acollision that does not exist here.
internal/packages, so no external consumer can importthem. 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.0milestone.internal/logandinternal/cryptoare the clearest possible namesfor what they hold. Renaming to something like
vlog/vcryptotosatisfy 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/idsorinternal/idtypeswould describe the ID/hash/path/credential wrappersbetter. 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,nolintlintdirectives in place. If you would rather rename
internal/typesspecifically, say so and I will schedule it after #73 lands rather than
before.