-
Notifications
You must be signed in to change notification settings - Fork 1
Separate public API from internal logic #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this 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 pull request refactors the stringifier architecture to separate public API concerns from internal implementation details. The Stringifier interface now provides a clean contract that always returns a string, while the new Handler interface supports the internal chain-of-responsibility pattern with nullable return types.
Changes:
- Introduced
Handlerinterface for internal chain-of-responsibility logic that can returnnull - Modified
Stringifierinterface to guarantee astringreturn type - Moved and renamed all stringifier implementations from
Stringifiers/toHandlers/namespace - Created
HandlerStringifierandDumpStringifieras public-facing implementations
Reviewed changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Stringifier.php | Updated interface to return only string (removed nullable return) |
| src/Handler.php | New interface for internal handlers with nullable return type |
| src/HandlerStringifier.php | New public stringifier that delegates to handlers with fallback |
| src/DumpStringifier.php | New fallback stringifier using print_r |
| src/Handlers/*.php | All handler implementations moved from Stringifiers namespace |
| src/Stringify.php | Removed (replaced by HandlerStringifier) |
| src/Stringifiers/CompositeStringifier.php | Removed (replaced by CompositeHandler) |
| tests/unit/Handlers/*.php | All tests updated to reflect Handler naming and namespace |
| tests/src/Double/FakeHandler.php | Test double updated but has signature mismatch |
| tests/src/Double/LameHandler.php | Test double properly updated to implement Handler |
| stringify.php | Helper function updated to use new HandlerStringifier |
| README.md | Documentation updated to reflect new API |
| phpcs.xml.dist | Configuration updated for new file paths |
Comments suppressed due to low confidence (1)
tests/src/Double/FakeHandler.php:21
- The method signature doesn't match the Handler interface. The Handler interface defines the return type as "string|null", but FakeHandler returns only "string". Additionally, the depth parameter has a default value here but not in the interface. For consistency with the Handler contract, the return type should be "string|null" and the default parameter should be removed.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
For a while now, our public `Stringifier` interface has been leaking internal control flow. By returning `string|null`, we were forcing consumers to deal with a "null" state that only existed to support our internal chain of responsibility. It made the public API clunky and less type-safe than it should be. I’ve resolved this by splitting the responsibilities into two distinct roles. The public `Stringifier` now provides a clean, guaranteed contract: it always returns a `string`. This removes the burden from the caller to handle nulls that they shouldn't have seen in the first place. Internally, the logic now lives in the `Handler` interface. This is where returning `null` still makes perfect sense—it’s how the chain-of-responsibility negotiates which handler takes the lead. By moving existing stringifiers into the `Handlers/` namespace, we’ve made a clear distinction between the implementation details and the public-facing service. The result is a more predictable API that hides its complexity without sacrificing the flexibility of the underlying pattern.
0c151a9 to
c082c6f
Compare
For a while now, our public
Stringifierinterface has been leaking internal control flow. By returningstring|null, we were forcing consumers to deal with a "null" state that only existed to support our internal chain of responsibility. It made the public API clunky and less type-safe than it should be.I’ve resolved this by splitting the responsibilities into two distinct roles. The public
Stringifiernow provides a clean, guaranteed contract: it always returns astring. This removes the burden from the caller to handle nulls that they shouldn't have seen in the first place.Internally, the logic now lives in the
Handlerinterface. This is where returningnullstill makes perfect sense—it’s how the chain-of-responsibility negotiates which handler takes the lead. By moving existing stringifiers into theHandlers/namespace, we’ve made a clear distinction between the implementation details and the public-facing service.The result is a more predictable API that hides its complexity without sacrificing the flexibility of the underlying pattern.