Skip to content

refactor: migrate zapcore.Level to RegisterIntEnum#117

Closed
leodido wants to merge 2 commits into
ld/register-int-enumfrom
ld/refactor-zapcore-register-int-enum
Closed

refactor: migrate zapcore.Level to RegisterIntEnum#117
leodido wants to merge 2 commits into
ld/register-int-enumfrom
ld/refactor-zapcore-register-int-enum

Conversation

@leodido
Copy link
Copy Markdown
Owner

@leodido leodido commented Apr 19, 2026 •
edited
Loading

Description

Replace the hand-written DefineZapcoreLevelHookFunc and StringToZapcoreLevelHookFunc with a single RegisterIntEnum[zapcore.Level](...) call, dogfooding the new enum registration infrastructure from #115/#116.

What changed

  • New enum_builtin.go: init() registers zapcore.Level via RegisterIntEnum with all 7 levels (debug through fatal).
  • Removed from internal/hooks/define.go: DefineZapcoreLevelHookFunc() function and its "zapcore.Level" registry entry. Removed zapcore import.
  • Removed from internal/hooks/decode.go: StringToZapcoreLevelHookFunc() function and its "zapcore.Level" registry entry. Removed zapcore import.
  • Updated tests: Fuzz test and type-guard integration test now use StringToIntEnumHookFunc directly. Removed stale flagcustom:"true" from zapcoreLevelOptions test struct.

Error message change

The error message for invalid zapcore levels changed from:

invalid string for zapcore.Level 'foo': ...

to:

invalid value "foo" for Level

This is user-visible for anyone matching on error strings.

Why not slog.Level?

slog.Level.UnmarshalText supports arithmetic offset syntax (DEBUG+1, INFO-2) that produces arbitrary integer values. A map-based RegisterIntEnum cannot express this, so slog.Level keeps its custom hooks.

Trade-off: hardcoded level map

The old hook delegated to zapcore.ParseLevel(). The new hook uses a hardcoded map of 7 levels. If a future zapcore version adds levels, enum_builtin.go must be updated. The level set has been stable since zapcore v1; a comment documents this.

Stacked on #116 → #115 → main.

How to test

go test ./...
cd examples/full && go test ./...
@leodido leodido added the enhancement New feature or request label Apr 19, 2026
@leodido leodido changed the title refactor: migrate zapcore.Level to RegisterIntEnum refactor: migrate zapcore.Level to RegisterIntEnum Apr 19, 2026
@leodido leodido force-pushed the ld/register-int-enum branch from e26e099 to f90cb75 Compare April 19, 2026 20:15
@leodido leodido force-pushed the ld/refactor-zapcore-register-int-enum branch from 8a836ad to 90453b6 Compare April 19, 2026 20:17
@leodido leodido mentioned this pull request Apr 19, 2026
Merged
@leodido leodido self-assigned this Apr 19, 2026
@leodido
Copy link
Copy Markdown
Owner Author

leodido commented Apr 19, 2026

Stack Review: PR #117

Verdict: ✅ Approve

Clean migration of zapcore.Level from hand-written hooks to RegisterIntEnum. Net -20 lines, removes zapcore import from both internal/hooks/define.go and internal/hooks/decode.go.

All findings from the initial review have been addressed in 90453b6:

  • 2.4 Error message change — Documented in commit message. The format changed from invalid string for zapcore.Level '...' to invalid value "..." for Level.
  • 2.5 zapcore.ParseLevel trade-off — Comment added to enum_builtin.go noting the hardcoded map and that it must be updated if zapcore adds levels.
  • 2.6 Stale flagcustom:"true" — Removed from zapcoreLevelOptions test struct. Tests pass without it, confirming the registry path works end-to-end.

The decision to keep slog.Level as a hand-written hook (due to arithmetic offset syntax like DEBUG+1) is well-reasoned and documented in the PR description.

All tests pass at the updated stack tip (90453b6).
@leodido leodido force-pushed the ld/register-int-enum branch from f90cb75 to de06cbd Compare April 19, 2026 20:35
leodido and others added 2 commits April 19, 2026 20:35
@leodido @ona-agent
refactor: migrate zapcore.Level to RegisterIntEnum
364bdbf 
Replace hand-written DefineZapcoreLevelHookFunc and
StringToZapcoreLevelHookFunc with a RegisterIntEnum[zapcore.Level]
call in enum_builtin.go init(). Removes the zapcore import from
internal/hooks/define.go and internal/hooks/decode.go.

slog.Level is left as-is because its UnmarshalText supports arithmetic
offsets (e.g. DEBUG+1) that a map-based approach cannot express.

Co-authored-by: Ona <no-reply@ona.com>
@leodido @ona-agent
fix: address review findings for zapcore migration
e5ea61c 
- Add comment noting hardcoded zapcore level map and its trade-off
  vs delegating to zapcore.ParseLevel (2.5)
- Remove stale flagcustom:"true" from zapcoreLevelOptions test struct;
  zapcore.Level is now in the registry and picked up automatically (2.6)

Note: the migration changes the error message format for invalid
zapcore levels from 'invalid string for zapcore.Level ...' to
'invalid value "..." for Level' (2.4).

Co-authored-by: Ona <no-reply@ona.com>
@leodido leodido force-pushed the ld/refactor-zapcore-register-int-enum branch from 90453b6 to e5ea61c Compare April 19, 2026 20:35
@leodido leodido deleted the branch ld/register-int-enum April 19, 2026 20:38
@leodido leodido closed this Apr 19, 2026
This was referenced Apr 19, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

1 participant

@leodido