Skip to content

fix: RecordToAttrsMap slice pre-allocation and FindAttrByGroupAndKey group traversal#37

Merged
samber merged 1 commit into
mainfrom
fix/record-to-attrs-map-and-finder
Mar 25, 2026
Merged

fix: RecordToAttrsMap slice pre-allocation and FindAttrByGroupAndKey group traversal#37
samber merged 1 commit into
mainfrom
fix/record-to-attrs-map-and-finder

Conversation

@samber
Copy link
Copy Markdown
Owner

@samber samber commented Mar 25, 2026

  • RecordToAttrsMap: change make([]slog.Attr, r.NumAttrs()) to
    make([]slog.Attr, 0, r.NumAttrs()) to avoid leading zero-valued
    elements that produce spurious empty-key entries
  • FindAttrByGroupAndKey: compare attrs[i].Key against groups[0]
    instead of key to correctly traverse into named groups

…group traversal

- RecordToAttrsMap: change make([]slog.Attr, r.NumAttrs()) to
  make([]slog.Attr, 0, r.NumAttrs()) to avoid leading zero-valued
  elements that produce spurious empty-key entries
- FindAttrByGroupAndKey: compare attrs[i].Key against groups[0]
  instead of key to correctly traverse into named groups
Copilot AI review requested due to automatic review settings March 25, 2026 02:08
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 25, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.15%. Comparing base (2da7764) to head (8c0e1a3).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #37      +/-   ##
==========================================
+ Coverage   95.34%   96.15%   +0.80%     
==========================================
  Files           4        4              
  Lines         258      260       +2     
==========================================
+ Hits          246      250       +4     
+ Misses         10        9       -1     
+ Partials        2        1       -1     
Flag Coverage Δ
unittests 96.15% <100.00%> (+0.80%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown

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

Fixes two small but user-visible correctness issues in slogcommon: map conversion from slog.Record could include spurious empty-key entries, and grouped attribute lookup could fail to traverse into named groups.

Changes:

  • Fix RecordToAttrsMap pre-allocation to avoid leading zero-valued slog.Attr entries.
  • Fix FindAttrByGroupAndKey to match on groups[0] (group name) and only return when a nested match is actually found.
  • Update/add tests to assert the corrected behavior for both cases.

Reviewed changes

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

File Description
attributes.go Pre-allocates attrs with length 0 and capacity r.NumAttrs() to prevent empty-key map entries.
attributes_test.go Turns the prior “bug documentation” into regression assertions (Len == 2, no "" key).
finder.go Corrects group traversal comparison (attrs[i].Key == groups[0]) and continues searching if a matching group doesn’t contain the key.
finder_test.go Updates cases to expect successful lookup inside a named group and adds a not-found case.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@samber samber merged commit 694d1fd into main Mar 25, 2026
13 checks passed
@samber samber deleted the fix/record-to-attrs-map-and-finder branch March 25, 2026 02:12
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.

2 participants