Conversation
WalkthroughThis pull request renames two public API methods on the Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Areas requiring attention:
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 0
🧹 Nitpick comments (3)
src/Condition.php (1)
130-146: API rename is clearer; consider deprecated aliases for BC + docblock tweakThe
decode()/encode()names are much clearer thanparse()/toString()and the implementations look unchanged. The only concern is potential breakage for existing callers that still use the old methods.If you expect any external usage of
Condition::parse()or$condition->toString(), consider keeping thin, deprecated shims to smooth the transition:@@ - /** - * Decode a JSON encoded condition string. - */ - public static function decode(string $payload): self + /** + * Decode a JSON encoded condition string. + * + * @throws ConditionException + */ + public static function decode(string $payload): self { try { $decoded = \json_decode($payload, true, flags: JSON_THROW_ON_ERROR); @@ return self::fromArray($decoded); } + + /** + * @deprecated Use Condition::decode() instead. + */ + public static function parse(string $payload): self + { + return self::decode($payload); + } @@ - /** - * Encode condition as JSON string. - * - * @throws ConditionException - */ - public function encode(): string + /** + * Encode condition as JSON string. + * + * @throws ConditionException + */ + public function encode(): string { @@ throw new ConditionException('Unable to encode condition: ' . $exception->getMessage()); } } + + /** + * @deprecated Use encode() instead. + */ + public function toString(): string + { + return $this->encode(); + }If this library is still pre‑1.0 or you explicitly plan a breaking release, you can of course skip the aliases and just keep the
@throwsannotation addition fordecode().Also applies to: 221-233
tests/ConditionTest.php (1)
184-189: Rename test to reflectdecode()rather thanparse()The test body now exercises
Condition::decode(), but the method name is stilltestParseRejectsInvalidJson(), which can be confusing when scanning the suite.Consider renaming it for clarity:
- public function testParseRejectsInvalidJson(): void + public function testDecodeRejectsInvalidJson(): void { $this->expectException(ConditionException::class); - Condition::decode('{"method":'); + Condition::decode('{"method":'); }Purely cosmetic, but keeps the terminology consistent with the new API.
README.md (1)
69-81: Docs correctly updated to encode/decode; optional terminology alignmentThe example and surrounding text now match the
encode()/decode()API, which is good. For extra consistency with the earlier bullet that mentions “JSON parsing helpers,” you might want to standardize the phrasing to “JSON encoding/decoding helpers” throughout, but that’s entirely optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(1 hunks)src/Condition.php(2 hunks)tests/ConditionTest.php(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/ConditionTest.php (1)
src/Condition.php (3)
encode(226-233)Condition(13-572)decode(133-146)
🔇 Additional comments (1)
tests/ConditionTest.php (1)
155-171: Round‑trip serialization test correctly updated to encode/decodeThe round‑trip now goes through
Condition::encode()andCondition::decode()and still asserts the same behavior. This keeps the test aligned with the new public API without changing semantics.
Summary by CodeRabbit
API Changes
toString()toencode()parse()todecode()Documentation
✏️ Tip: You can customize this high-level summary in your review settings.