Skip to content

feat: type key-values only#2

Merged
rjfonseca merged 1 commit into
mainfrom
feat/typed-keyvalues-only
May 29, 2025
Merged

feat: type key-values only#2
rjfonseca merged 1 commit into
mainfrom
feat/typed-keyvalues-only

Conversation

@rjfonseca
Copy link
Copy Markdown
Member

Allowing for arbitrary lists of key-values was causing unintentional panicking when the developer mistakes the type of a value. E.g. using an error code with type string instead of errors.Code.

By Allowing only typed values to be used we eliminate one potential panic.

Allowing for arbitrary lists of key-values was causing unintentional
panicking when the developer mistakes the type of a value. E.g. using an
error code with type string instead of errors.Code.

By Allowing only typed values to be used we eliminate one potential
panic.
Copilot AI review requested due to automatic review settings May 23, 2025 21:04
@rjfonseca rjfonseca requested a review from xico42 as a code owner May 23, 2025 21:04
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR restricts the errors.With API to accept only typed key-value arguments, preventing panics from incorrect varargs use.

  • Changed With signature to only accept KeyValuer types and removed varargs parsing helpers.
  • Updated tests to use the new KeyValuer-only interface.
  • Revised README and example code to reflect the new usage model.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
with.go Updated With signature, removed argsToKeyValues and cutKV.
with_test.go Replaced generic varargs tests with KeyValuer-only tests.
op_test.go Adjusted test to pass Op directly instead of splitting key/value.
README.md Expanded usage section and examples to show KeyValuer calls.
Comments suppressed due to low confidence (4)

with_test.go:18

  • [nitpick] Consider renaming this to TestWithNilErrorReturnsNil or TestWith_NilErr_ReturnsNil for clearer intent about testing the nil-err case.
func TestWithNoError(t *testing.T) {

with_test.go:25

  • A test for the case where a non-nil error is passed with zero KeyValuer arguments (i.e. errors.With(rootErr)) would ensure the function still returns the original error.
func TestWith(t *testing.T) {

with.go:14

  • [nitpick] Consider making this panic message more descriptive, e.g. include the key's type or change it to panic("key type is not comparable") to aid debugging.
panic("key is not comparable")

README.md:10

  • The example uses fmt.Errorf but the fmt package isn't imported in the snippet. Add "fmt" to the import block for the example to compile as shown.
import (

@rjfonseca rjfonseca merged commit 3fd9904 into main May 29, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants