Fix #99: Add customizable context format and value conversion#140
Fix #99: Add customizable context format and value conversion#140WarLikeLaux wants to merge 8 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #140 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 165 174 +9
===========================================
Files 11 12 +1
Lines 454 486 +32
===========================================
+ Hits 454 486 +32 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds new configuration hooks to customize how log context is rendered and how context values are converted to strings, enabling user-defined context ordering and formatting while keeping the default output unchanged.
Changes:
- Add
setConvertToString(),setContextTemplate(), andsetContextFormat()APIs onTarget/Formatterand implement precedence rules in formatting. - Update formatter context assembly to support template-based section ordering and fully custom rendering via callable.
- Add/extend tests and documentation covering the new customization options.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Message/Formatter.php |
Implements custom context formatting/template logic and pluggable value-to-string conversion. |
src/Target.php |
Exposes new formatter customization methods at the target level. |
tests/Message/FormatterTest.php |
Adds coverage for new formatter customization behaviors and precedence/error cases. |
tests/TargetTest.php |
Adds target-level tests for new customization methods. |
tests/TestAsset/DummyTarget.php |
Ensures test target mirrors new formatter customization settings for exported formatting. |
README.md |
Documents new formatting customization options and examples. |
CHANGELOG.md |
Records the new feature in the upcoming release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@WarLikeLaux resolve conflicts, please |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR extends the logging system with three new customization methods. ChangesContext formatting and value conversion customization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/TargetTest.php (1)
261-320: ⚡ Quick winCover the non-exported path for these new setters.
All three tests go through
collectOneAndExport(), so they only exerciseDummyTarget::$exportFormatter. If a future change stopped delegating toparent::setContextFormat(),parent::setContextTemplate(), orparent::setConvertToString(), these tests would still pass while buffered, non-exported formatting regressed. Please add acollect(..., false)assertion here or a dedicated test for the non-exported path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/TargetTest.php` around lines 261 - 320, Tests only exercise the exported path via collectOneAndExport() and DummyTarget::$exportFormatter; add coverage for the non-exported (buffered) path by invoking collect(..., false) or a dedicated test that calls collect with export=false and then asserting the buffered formatting result (e.g. call formatMessages() or the buffer accessor) to match the same expected strings; update the tests testSetConvertToString, testSetContextTemplate, and testSetContextFormat (or add a new test) to call collect($level, $message, $context, false) or collect(..., false) after configuring setConvertToString, setContextTemplate, and setContextFormat so you verify parent::setConvertToString / parent::setContextTemplate / parent::setContextFormat behavior for the non-exported path as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Message/Formatter.php`:
- Around line 298-299: The current return expression in Formatter.php appends a
trailing "\n" unconditionally, causing a stray newline when $messageSection and
$commonContextString (and trace) are empty; update the return so the trailing
newline is only added when there is at least one non-empty context piece (i.e.
when $messageSection !== '' || $commonContextString !== '' || trace exists).
Concretely, adjust the return that uses $messageSection and $commonContextString
to concatenate the separator/newline only when one of those variables (or the
trace output) is non-empty so the method returns an empty string when there is
no context. Ensure you reference and use the existing $messageSection and
$commonContextString variables (and the trace check used earlier in this method)
when constructing the final string.
---
Nitpick comments:
In `@tests/TargetTest.php`:
- Around line 261-320: Tests only exercise the exported path via
collectOneAndExport() and DummyTarget::$exportFormatter; add coverage for the
non-exported (buffered) path by invoking collect(..., false) or a dedicated test
that calls collect with export=false and then asserting the buffered formatting
result (e.g. call formatMessages() or the buffer accessor) to match the same
expected strings; update the tests testSetConvertToString,
testSetContextTemplate, and testSetContextFormat (or add a new test) to call
collect($level, $message, $context, false) or collect(..., false) after
configuring setConvertToString, setContextTemplate, and setContextFormat so you
verify parent::setConvertToString / parent::setContextTemplate /
parent::setContextFormat behavior for the non-exported path as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1bf644a5-c169-46ad-a11c-5e03017d83f4
📒 Files selected for processing (9)
CHANGELOG.mdREADME.mdsrc/Message/Formatter.phpsrc/Message/VarDumperValueConverter.phpsrc/Target.phptests/Message/FormatterTest.phptests/Message/VarDumperValueConverterTest.phptests/TargetTest.phptests/TestAsset/DummyTarget.php
|
Waiting #146 |
|
@WarLikeLaux would you please rebase it? |
|
@samdark done 🫡 |
What does this PR do?
Add two constructor parameters to
Target(and its subclassesStreamTarget,PsrTarget) for customizing context output:$contextFormat(string|callable|null) - a template string for reordering context sections, or a callable for full control over context rendering$stringConverter(?callable) - custom value-to-string conversion (defaults to VarDumper-based conversion)Use cases
$stringConverterreplaces the default VarDumper-based conversion, for example to JSON-encode all context values:A
$contextFormattemplate string reorders sections via{trace},{message},{common}placeholders. Empty sections are omitted automatically:A
$contextFormatcallable gives full control over context rendering, including custom headers:$contextFormataccepts either a template string or a callable. Default output is unchanged when neither parameter is passed.Summary by CodeRabbit
New Features
Documentation